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

feat(number-format): Add duration formatter with colon notation #30593

Merged

Conversation

gerbermichi
Copy link
Contributor

@gerbermichi gerbermichi commented Oct 14, 2024

SUMMARY

Introduces a new formatter, DURATION_COL, for durations presented in colon notation (e.g., 0:10.5).

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

image

TESTING INSTRUCTIONS

  1. Create a chart with a duration (in ms) column
  2. Change the number formatter to Duration in ms (10500 => 0:10.5)

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@dosubot dosubot bot added the change:frontend Requires changing the frontend label Oct 14, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@gerbermichi gerbermichi force-pushed the gerbermichi/time-duration-formatter branch from d0143f8 to 3e22d7b Compare October 14, 2024 09:09
@michael-s-molina michael-s-molina added the review:checkpoint Last PR reviewed during the daily review standup label Oct 14, 2024
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

We don't need to create a new formatter for this, as the existing duration formatter already supports this functionality. Also, let's stick to milliseconds for now to keep this aligned with the existing duration formatters.

Comment on lines 22 to 43
export default function createTimeDurationFormatter(
config: {
description?: string;
id?: string;
label?: string;
multiplier?: number;
} = {},
) {
const { description, id, label } = config;

return new NumberFormatter({
description,
formatFunc: value => {
const v = Math.abs(Math.round(value));
const h = Math.floor(v / 60);
const m = v % 60;
return `${h}:${String(m).padStart(2, '0')}`;
},
id: id ?? 'duration_format',
label: label ?? `Duration formatter`,
});
}
Copy link
Member

Choose a reason for hiding this comment

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

We already have a dependency on pretty-ms that achieves this and (see colonNotation option).

@@ -78,6 +79,7 @@ export default function setupFormatters(
'DURATION_SUB',
createDurationFormatter({ formatSubMilliseconds: true }),
)
.registerValue('TIME_DURATION', createTimeDurationFormatter())
Copy link
Member

@villebro villebro Oct 14, 2024

Choose a reason for hiding this comment

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

Please check the README.md in the number-format directory for an example how to implement this. I believe this should do it:

Suggested change
.registerValue('TIME_DURATION', createTimeDurationFormatter())
.registerValue('DURATION_COL', createTimeDurationFormatter({ colonNotation: true }))

Copy link
Contributor Author

@gerbermichi gerbermichi Oct 15, 2024

Choose a reason for hiding this comment

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

Thank you. This does not the same, it will format it in yy:dd:hh:mm:ss and I would like to create a formatter for hh:mm. However, I can add both, DURATION_COL and DURATION_COL_SHORT. What are your thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

@gerbermichi as pretty-ms already supports colon notation, has good test coverage, and lots of customization options, I suggest looking into how this could be implemented with that library. Did you try adding secondsDecimalDigits: 0 to remove the seconds? See the unit tests for some typical use cases: https://github.com/sindresorhus/pretty-ms/blob/d00183f8a040315005452f6f6dec30c581b258a2/test.js#L312-L380 If pretty-ms still doesn't support what you're looking for, I suggest opening a PR on that repo, as that will also benefit the general OSS community.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@villebro thank you for your feedback. I changed this PR so that it uses the colonNotation from ´pretty-ms´.

@villebro
Copy link
Member

FYI: I opened a PR to bump pretty-ms to pull in a critical fix for colonNotation, as that's what we should be using here: #30599

@michael-s-molina michael-s-molina removed the review:checkpoint Last PR reviewed during the daily review standup label Oct 15, 2024
Introduces a new formatter, DURATION_COL, for durations presented in colon notation (e.g., 0:10.5). This update enhances the flexibility of duration display formats within the application.
@gerbermichi gerbermichi force-pushed the gerbermichi/time-duration-formatter branch from 3e22d7b to 8b7cf05 Compare October 16, 2024 08:46
@gerbermichi gerbermichi changed the title Add time duration formatter (Duration in m (61 => 1:01)) feat(D3_FORMAT_OPTIONS): Add duration formatter with colon notation Oct 16, 2024
@gerbermichi gerbermichi requested a review from villebro October 24, 2024 07:33
@villebro villebro changed the title feat(D3_FORMAT_OPTIONS): Add duration formatter with colon notation feat(number-format): Add duration formatter with colon notation Oct 24, 2024
@villebro villebro merged commit 3d443e0 into apache:master Oct 24, 2024
39 of 41 checks passed
nyohasstium pushed a commit to Webgains/superset that referenced this pull request Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:frontend Requires changing the frontend packages size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants