v1.0 of The Final Count Down plugin #16
Conversation
|
Hi Tom, we're really sorry for how absurdly long this has been sitting ready for review. I'll review it now. |
|
Hey Alan, per our agreement with Tom, he does not need to wait for us to review plugins that aren't referenced directly from the product. If you have the bandwidth, then go for it! But not necessary any longer. Feel free to respond with a "Merge" meme at your earliest convenience. |
addelong
left a comment
addelong
left a comment
There was a problem hiding this comment.
This looks good Tom, I think the main thing is that I'd like for us to just use the countdown() events directly instead of triggering another event.
| this.paddingZeros = paddingZeros || 2; | ||
|
|
||
| // Register the units we will be displaying with the CountDownClock | ||
| this.countDownClock.addUnit(unitsToDisplay); |
| this.unitsToDisplay = unitsToDisplay; | ||
|
|
||
| // The number of 0s to add our value with, ie "1" becomes "01" if the value is 2 | ||
| this.paddingZeros = paddingZeros || 2; |
There was a problem hiding this comment.
So this should probably be called something like paddedLengthOfNumber, because it's not actually the number of zeroes.
| // The date we're counting down to | ||
| this.date = date; | ||
|
|
||
| // bitwise operator containing the units to be displayed, ie: HOURS, SECONDS, etc |
There was a problem hiding this comment.
This comment confuses me, I don't know what the type of this variable is supposed to be.
| this.assertValidUnit(unitConstant); | ||
|
|
||
| // If this unit hasn't already been added | ||
| if (this.includedUnits.indexOf(unitConstant) < 0) { |
There was a problem hiding this comment.
I think it would be easier to just have one units instance variable that is an array of strings, and then transform it into an OR'd binary value at the least responsible moment, aka when you instantiate the countdown().
|
|
||
| function(newTimeSpan){ | ||
| self.trigger( | ||
| CountDownClock.EVENTS.TICK, |
There was a problem hiding this comment.
I think this is my biggest issue with the PR - triggering your own events in response to other events is kind of an anti-pattern. I think we should get rid of the MicroEvent module, and just pass in the object that needs to be notified when we get a countdown tick.
| * | ||
| * @param {string} unit | ||
| */ | ||
| assertValidUnit: function(unit) { |
There was a problem hiding this comment.
COULD combine this function with isValidUnit.
| 2. Tag each of the Text Boxes created in the above step with `count-down` | ||
| 3. Set the payload of each box to a value from the list below (including capitalisation), based on the units it will display. |
There was a problem hiding this comment.
I'm curious, we do a toLowerCase() on the units in the code, why is capitalization important?
Resolves ceros/ceros-product#2687
A reusable version of the count down code we have implemented for a number of customers.
See README.md for instructions on use.