Update TTL picker on add replication secondary#9271
Conversation
This change updates the TTL picker to the new version to match most updated designs. The component also allows the default value to be more obvious
chelshaw
force-pushed
the
ui/replication-secondary-ttl
branch
from
8ca25e3 to
e037544
Compare
| @helperTextDisabled="If not set, the default value (30 minutes) will be used" | ||
| @helperTextEnabled="After this period, the generated token will no longer be valid." | ||
| @onChange={{action "updateTtl"}} | ||
| @changeOnInit={{true}} |
There was a problem hiding this comment.
Added this param because I found that if you make a secondary with a custom TTL, and then immediately make another secondary without touching the TTL picker, it uses the previous value instead of the actual default (which is passed here as initialValue
Monkeychip
left a comment
Monkeychip
left a comment
There was a problem hiding this comment.
Address a couple of the questions and you should be good. I think this get's backported because they cut the release branch for 1.5, but you can confirm if you see the release/1.5 branch on master
| This is the Time To Live for the generated secondary token. After this period, the generated token will no longer be valid. | ||
| </p> | ||
| <TtlPicker2 | ||
| @initialValue="30m" |
There was a problem hiding this comment.
Judging from the code here I'd expect to see the initial value as 30 minutes, but for some reason I'm seeing 1800 seconds in the UI. I know these values are equal, but everywhere else in the UI we default to "minutes" as the unit. If possible it'd be great to fix this so it's consistent.
There was a problem hiding this comment.
This is expected behavior for the current design, but not ideal so I went ahead and updated it (with test coverage!) so that the unit matches what is passed in on the value
There was a problem hiding this comment.
I updated the TTL picker UI to match the unit passed in, but the unit being passed to the API is being standardized to seconds for a couple reasons:
- Some of the APIs expect units, and others don't (defaulting to seconds), so the new TTL picker was designed to account for either of those
- None of the APIs accept 'days' as a unit, but we provide that as a dropdown option, so always converting to seconds when we send to the API ensures that we are consistent at that level.
There was a problem hiding this comment.
awesome, thank you for tracking that down and adding more tests! i think the explanation makes sense.
unfortunately in testing this though i found another bug when adding a secondary:
- toggle the TTL picker on
- enter a value (i.e. 3 minutes)
- toggle the TTL picker off
- the value from step 2 still gets sent to the API 🙈
let me know if you need any help!
andaley
left a comment
andaley
left a comment
There was a problem hiding this comment.
had one request since it looks like the initial value isn't working as expected; otherwise everything looks great!
* Update TTL picker on add replication secondary This change updates the TTL picker to the new version to match most updated designs. The component also allows the default value to be more obvious * Remove erroneous else * Add changeOnInit param for TtlPicker2 and use it on add secondary page * Update ttlPicker2 docs and add tests for new param * Calculate value in unit provided on init for ttl-picker2, with tests * Cleanup and make ttl-picker2 test more specific
This change updates the TTL picker to the new version to match most updated designs. The component also allows the default value to be more obvious * Remove erroneous else * Add changeOnInit param for TtlPicker2 and use it on add secondary page * Update ttlPicker2 docs and add tests for new param * Calculate value in unit provided on init for ttl-picker2, with tests * Cleanup and make ttl-picker2 test more specific
* Update TTL picker on add replication secondary This change updates the TTL picker to the new version to match most updated designs. The component also allows the default value to be more obvious * Remove erroneous else * Add changeOnInit param for TtlPicker2 and use it on add secondary page * Update ttlPicker2 docs and add tests for new param * Calculate value in unit provided on init for ttl-picker2, with tests * Cleanup and make ttl-picker2 test more specific
4 participants
This change updates the TTL picker to the new version to match most updated designs. The component also allows the default value to be more obvious.
BEFORE
AFTER (default)
AFTER (enabled/custom)
