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

Move some datetime functions to @openshift-console/plugin-shared #12037

Merged
merged 1 commit into from Sep 28, 2022

Conversation

kyoto
Copy link
Member

@kyoto kyoto commented Sep 13, 2022

Move formatPrometheusDuration and parsePrometheusDuration to @openshift-console/plugin-shared

@openshift-ci openshift-ci bot added component/ceph Related to ceph-storage-plugin component/core Related to console core functionality approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console component/monitoring Related to monitoring component/pipelines Related to pipelines-plugin labels Sep 13, 2022
export const toISODateString = (date: Date): string =>
`${date.getFullYear()}-${zeroPad(date.getMonth() + 1)}-${zeroPad(date.getDate())}`;

export const twentyFourHourTime = (date: Date, showSeconds?: boolean): string => {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason plugins shouldn't use the standard Intl.DateTimeFormat for this instead of writing a separate utility? Something like

new Intl.DateTimeFormat(
  'en',
  {
     hour: 'numeric',
     minute: 'numeric',
     second: 'numeric',
     hourCycle: 'h23',
  }
 ).format(date);

Then we can format for the user's locale as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @spadgett. I removed toISODateString and twentyFourHourTime from this PR and will address them with a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

PR for cleaning up the use of toISODateString and twentyFourHourTime: #12053

@@ -1 +1,2 @@
export * from './components';
export * from './utils';
Copy link
Member

Choose a reason for hiding this comment

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

Can I suggest a package like datetime that has a more specific meaning instead of a generic utils package?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

const w = d * 7;
const units = { w, d, h, m, s };

// Formats a duration in milliseconds like "1h 10m"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we're generating docs for console-plugin-shared yet, but it would be good to add ts-doc here for when we do.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Move formatPrometheusDuration and parsePrometheusDuration to
@openshift-console/plugin-shared
@kyoto
Copy link
Member Author

kyoto commented Sep 22, 2022

/retest

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2022
@kyoto
Copy link
Member Author

kyoto commented Sep 23, 2022

This impacts the user facing API, so requesting approvals.

/cc @RickJWagner
For PX approval

/cc @opayne1
For docs approval

Copy link

@RickJWagner RickJWagner left a comment

Choose a reason for hiding this comment

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

No complaints from PLM.

@RickJWagner
Copy link

/label px-approved

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Sep 23, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 23, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kyoto, RickJWagner, spadgett

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@opayne1
Copy link
Contributor

opayne1 commented Sep 23, 2022

/label docs-approved

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Sep 23, 2022
@Tai-RedHat
Copy link

test PR with cluster bot, no other regression issues.
/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Sep 27, 2022
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 2aaaa39 and 2 for PR HEAD 673d272 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 15b885d and 1 for PR HEAD 673d272 in total

@openshift-merge-robot openshift-merge-robot merged commit 35bf511 into openshift:master Sep 28, 2022
@kyoto kyoto deleted the sdk-expose-datetime branch September 29, 2022 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/dev-console Related to dev-console component/monitoring Related to monitoring component/pipelines Related to pipelines-plugin docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants