Explicitly set seconds to 0 from selector #4559
Conversation
| store.setConditionDateTime('testingDateTime'); | ||
| expect(store.condition.time).to.equal('testingDateTime'); | ||
| it('sets the datetime', () => { | ||
| const testTime = new Date('1973-06-11 07:52'); |
There was a problem hiding this comment.
Can we test that it actually sets the seconds to 0 ?
There was a problem hiding this comment.
What about this?
const testTime = new Date('1973-06-11 07:52:17');There was a problem hiding this comment.
Sure, can test additionally.
js/src/ui/GasPriceEditor/store.js
Outdated
| @@ -104,6 +104,9 @@ export default class GasPriceEditor { | |||
| } | |||
|
|
|||
| @action setConditionDateTime = (time) => { | |||
| time.setMilliseconds(0); // ignored by/not passed to Parity | |||
There was a problem hiding this comment.
Potentially dangerous as it mutates time instead of cloning it using new Date(time).
There was a problem hiding this comment.
Verified that it doesn't affect the either the MUI date or time components used with manual testing prior to pushing (and initial implementation).
These two separate UI components actually operate on the same value, both use this setter in their respective updates - this was the actual original implementation concern, i.e. do we need to maintain 2 separate values and combine them.
Cloning for future-proofing though, since we are looking at different date-picker components already due to the obtrusive nature of the current "standard" ones.
ngotchac
added
A5-grumble 🔥
jacogr
added
A0-pleasereview 🤓
ngotchac
added
A8-looksgood 🦄
* Explicitly set seconds/milli to 0 * Use condition time & block setters consistently * Fix failing test * test for 0 ms & sec * It cannot hurt, clone date before setting * Prettier date test constants (OCD)
Be explicit rather than assuming that timeselector zeros seconds. Sets -
If we do change the time selector component to be granular, the seconds setting can be removed.