area/ui: Support typing multiple units in relative range picker#5006
Conversation
|
🤖 Meticulous spotted visual differences in 2 of 637 screens tested: view and approve differences detected. Last updated for commit b03417a. This comment will update as new commits are pushed. |
yomete
changed the title
metalmatze
left a comment
metalmatze
left a comment
There was a problem hiding this comment.
Looks really good!
I wonder if our life could be easier if this abstraction could simple work on seconds or milliseconds (which is what JS uses) rather than floating point numbers? Maybe I'm missing some context and what I'm saying doesn't make sense.
|
|
||
| describe('parseInput', () => { | ||
| it('should parse single unit inputs correctly', () => { | ||
| expect(parseInput('30s')).toEqual({value: 30, unit: UNITS.SECOND}); |
There was a problem hiding this comment.
What's going to happen for something like 90s vs 1m30s? Are both parsed as 90s?
| expect(parseInput('30s')).toEqual({value: 30, unit: UNITS.SECOND}); | |
| expect(parseInput('90s')).toEqual({value: 90, unit: UNITS.SECOND}); |
There was a problem hiding this comment.
Good questions.
To answer the test question (and maybe the first one) internally, and as a request to the backend they are both processed and parsed as 90s but in the UI component, it will be parsed as 1m30s, and this is because the logic favours the higher unit over the lower ones.
Regarding the suggestion to work with seconds or milliseconds instead of floating-point numbers, the floating point numbers are only being used to format the human readable text that shows up in the RelativeDatepicker component. This allows us to preserve the user's input format when possible. But as mentioned, we still send the timerange in milliseconds to the backend.
If you think there might be advantages to using milliseconds throughout the entire component, I'm open to discussing potential improvements or alternative implementations.
There was a problem hiding this comment.
I don't have a strong opinion on how it gets represented internally. I was thinking that maybe there was an easier way. Seems like it's already pretty good, so please ignore my suggestion.
There was a problem hiding this comment.
Feel free to merge this as is too.
2 participants
Fixes #4986