Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix bid dropdown dismissal #1230

Merged
merged 3 commits into from
Apr 18, 2017
Merged

fix bid dropdown dismissal #1230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
43230ed
fix bid dropdown dismissal
acjay Apr 13, 2017
b372941
dont delay first call for update
acjay Apr 13, 2017
0c5d91a
fix test
acjay Apr 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions desktop/apps/artwork/components/auction/test/view.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@ Backbone = require 'backbone'

describe 'auction', ->
before (done) ->
sinon.stub global, 'setInterval'
sinon.stub global, 'setTimeout'
benv.setup ->
benv.expose $: benv.require('jquery'), jQuery: benv.require('jquery')
Backbone.$ = $
done()

after ->
benv.teardown()
global.setInterval.restore()
global.setTimeout.restore()

beforeEach ->
@ArtworkAuctionView = benv.requireWithJadeify(
Expand Down
21 changes: 17 additions & 4 deletions desktop/apps/artwork/components/auction/view.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{ defer, extend, before } = require 'underscore'
{ defer, extend, before, isEqual } = require 'underscore'
Backbone = require 'backbone'
{ AUCTION, CURRENT_USER } = require('sharify').data
Form = require '../../../../components/form/index.coffee'
Expand All @@ -11,6 +11,9 @@ helpers = require './helpers.coffee'
metaphysics = require '../../../../../lib/metaphysics.coffee'
template = -> require('./templates/index.jade') arguments...

LOT_STANDING_MAX_POLLS = 10
LOT_STANDING_POLL_INTERVAL = 1000
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My version of the Damassi Transform is giving names to magic numbers 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is definitely clearer 👍


module.exports = class ArtworkAuctionView extends Backbone.View
className: 'artwork-auction'

Expand All @@ -23,7 +26,9 @@ module.exports = class ArtworkAuctionView extends Backbone.View
'change .js-artwork-auction-max-bid': 'setMaxBid'

initialize: ({ @data }) -> #
setInterval before(10, @updateBidLabel), 1000
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to change this pattern, because the way it was before, the timer keeps on firing. It's better to clean up after ourselves by scheduling only the number of calls we want to make, rather than squelch the function calls beyond the ones we want.

# Defer until next tick. This really just gets the sequence right for
# testing to work. This doesn't feel like a great justification, though.
setTimeout (=> @updateBidLabel LOT_STANDING_MAX_POLLS), 0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The before hook in the tests uses a __set__ mechanism I'm not sure I understand to affect the value of AUCTION in the scope of the class, but this only takes effect after instantiation of the class. I work around this by deferring to next tick. Open to better suggestions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should get you what you need: https://github.com/artsy/force/blob/master/desktop/apps/artwork/client/test/index.coffee#L18 -- benv's expose function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so maybe if I add artwork_id to it, it'll work. I'll try!


openBuyersPremium: (e) ->
e.preventDefault()
Expand Down Expand Up @@ -102,7 +107,7 @@ module.exports = class ArtworkAuctionView extends Backbone.View
@$('.js-artwork-auction-bid-button')
.text "Bid #{$(e.target).find('option:selected').attr 'data-display'}"

updateBidLabel: =>
updateBidLabel: (remaining) ->
metaphysics
query: """
query artwork($id: String!, $sale_id: String!) {
Expand All @@ -125,7 +130,15 @@ module.exports = class ArtworkAuctionView extends Backbone.View
variables: id: AUCTION.artwork_id, sale_id: AUCTION.id
req: user: CURRENT_USER
.then (data) =>
old_data = @data
@data = extend data, user: CURRENT_USER?
@render()

@render() unless isEqual(old_data, data)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently isEqual does a deep compare, which is handy here. It's still not great that we re-render a component that the user might be in the middle of interacting with. But comparing the data driving the render seemed like the easiest partial fix.


@delayedUpdateBidLabel(remaining - 1) if remaining > 0

.catch console.error.bind console

delayedUpdateBidLabel: (remaining) ->
setTimeout (=> @updateBidLabel remaining), LOT_STANDING_POLL_INTERVAL