Skip to content
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: updates info table row jsdoc #20697

Merged
merged 18 commits into from
May 30, 2023
Merged

Conversation

hellobontempo
Copy link
Contributor

@hellobontempo hellobontempo commented May 19, 2023

TTL duration values will now universally read with their expanded unit, instead of the letter abbreviation. For example, 30 seconds instead of 30s

Screenshot 2023-05-22 at 12 40 27 PM

Copy link
Contributor

@kiannaquach kiannaquach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is so much better! 💖 thanks for updating it to use format-duration

@hellobontempo hellobontempo enabled auto-merge (squash) May 22, 2023 19:41
@hellobontempo hellobontempo removed this from the 1.14 milestone May 23, 2023
@hellobontempo hellobontempo disabled auto-merge May 23, 2023 22:44
@@ -6,23 +6,17 @@
import { helper } from '@ember/component/helper';
import { formatDuration, intervalToDuration } from 'date-fns';

export function duration([time], { nullable = false }) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nullable = false was only used by transit, which is removed now

@hellobontempo hellobontempo added this to the 1.15 milestone May 25, 2023
Copy link
Contributor

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes the API returns Duration strings (like 10m) and sometimes it turns a number, which is the duration value in seconds. I think if we want to update the TTL values to the full unit we should do it consistently for durations like 10m as well.
In the TTL picker component we use the library @icholy/duration to parse the given duration if it exists. This might be a good moment to pull some of that parsing into a util that can be shared between the display helper and the component for choosing the TTL

return Duration.parse(duration).seconds();
} catch (e) {
// return false so parent can decide how to handle parsing error
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parent should probably check === some value rather than falsey (!returnedSeconds) since the valid duration 0 would be falsey. So I would return null instead but that's just me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went back and forth between having null here and couldn't decide since they're both false. But that's a good point! Thank you

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

Copy link
Contributor

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@hellobontempo hellobontempo merged commit 0615a50 into main May 30, 2023
@hellobontempo hellobontempo deleted the ui/update-info-table-row-jsdoc branch May 30, 2023 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants