-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
UI: TTL picker cleanup #18114
UI: TTL picker cleanup #18114
Conversation
2678da1
to
88b6210
Compare
|
||
module('Integration | Component | ttl picker', function (hooks) { | ||
module('Integration | Component | ttl-picker', function (hooks) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Contents of the test from ttl-picker2
replaced this test for a deleted component
* automatically recalculates the time value when unit is updated unless time has been changed recently. | ||
* Once all instances of TtlPicker are replaced with TtlPicker2, this component will be removed and | ||
* TtlPicker2 will be renamed to TtlPicker. | ||
* TtlPicker components are used to enable and select duration values such as TTL. This component renders a toggle by default and: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Contents of TtlPicker2
component replaced the unused TtlPicker
component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great removing the extra ttl component and all the glimmerizing thank you! I'm a bit confused on the ttl-form
component and whether or not we could get down to a single component, or at least extend the TtlPicker class perhaps for the form component?
// initialValue: null, | ||
// changeOnInit: false, | ||
// hideToggle: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to keep these commented out values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean with the template being less than ideal when incorporating the toggle but I think it's still worth it to get rid of the form component. Thanks for doing that! It's too bad that the
Toggle
component has to wrap everything. I thought that it would be standalone similar to a checkbox.
Yeah, it doesn't have to wrap everything, but in order to get behavior where you can click on the label to toggle the content, it does have to wrap. A future update could be something where the parent can override the input ID so that we can match on a custom label, but I think that's out of scope for now.
@@ -108,29 +113,29 @@ export default Component.extend({ | |||
time = convertFromSeconds(seconds, unit); | |||
} | |||
} catch (e) { | |||
// if parsing fails leave as default 30s | |||
// if parsing fails leave it empty | |||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to avoid setting unit
and enableTTl
if the time parsing fails by returning here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean with the template being less than ideal when incorporating the toggle but I think it's still worth it to get rid of the form component. Thanks for doing that! It's too bad that the Toggle
component has to wrap everything. I thought that it would be standalone similar to a checkbox.
* UI: TTL picker cleanup (#18114) * Update config-pki.hbs Unable to make Pki changes because we don't have action handleCrlTtl * fix space * remove test --------- Co-authored-by: Chelsea Shaw <82459713+hashishaw@users.noreply.github.com>
This PR does some cleanup with the TTL components. This also finishes up a long-lived project to replace all
<TtlPicker>
components with<TtlPicker2>
which was developed to replace it, and includes a toggle to enable.TtlPicker
withTtlPicker2
TtlPicker
,TtlForm
,TtlPicker2
) into one componentBecause there are so many moving pieces and similarly named files, I highly recommend reviewing this by stepping through the individual commits
TTL Picker with

@hideToggle
TTL Picker
