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: introduced time component #5485

Closed
wants to merge 5 commits into from
Closed

Conversation

ovflowd
Copy link
Member

@ovflowd ovflowd commented Jul 9, 2023

Description

This PR introduces a simple Time component that fulfils the specification presented on the issue.

It also updates the en locale to use en-GB

Related Issues

Fixes #5481

@ovflowd ovflowd requested a review from a team as a code owner July 9, 2023 09:23
@vercel
Copy link

vercel bot commented Jul 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 10, 2023 2:33pm
nodejs-org-stories ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 10, 2023 2:33pm

@vercel vercel bot temporarily deployed to Preview – nodejs-org-stories July 9, 2023 09:24 Inactive
@ovflowd ovflowd changed the title chore: updated english to use en-GB locale feat: introduced time component Jul 9, 2023
Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

You are recoding an fonctionnality of react-intl (formattedDate)

@ovflowd
Copy link
Member Author

ovflowd commented Jul 9, 2023

You are recoding an fonctionnality of react-intl (formattedDate)

I'm not. Why do you think this component is reimplementing existing functionality from react-intl?

Edit: I see your point; refer to my comment on #5481 (comment)

@ovflowd
Copy link
Member Author

ovflowd commented Jul 9, 2023

I'm updating this PR to use FormattedDate, and I want to clarify that we're not "redoing/reimplementing" existing functionality. It's often more beneficial to utilize Native JavaScript APIs instead of relying on sugarcoated components.

Additionally, it's important to note that the Time component acts as a "wrapper" for HTML's time element. It doesn't replace react-intl's Date & Time Components, which return a plain JSX-string.

I'm providing these explicit details to ensure you understand why your assessment may not be accurate, even though using FormattedDate makes sense in this case.

components/Common/Time/index.tsx Outdated Show resolved Hide resolved
components/Common/Time/index.tsx Outdated Show resolved Hide resolved
components/Common/Time/index.tsx Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – nodejs-org-stories July 10, 2023 09:44 Inactive
@vercel vercel bot temporarily deployed to Preview – nodejs-org July 10, 2023 09:45 Inactive
@ovflowd ovflowd force-pushed the feat/generic-time-component branch from 5e9509b to fbb5a01 Compare July 10, 2023 09:51
@vercel vercel bot temporarily deployed to Preview – nodejs-org-stories July 10, 2023 09:58 Inactive
@vercel vercel bot temporarily deployed to Preview – nodejs-org July 10, 2023 09:58 Inactive
@vercel vercel bot temporarily deployed to Preview – nodejs-org-stories July 10, 2023 14:32 Inactive
@vercel vercel bot temporarily deployed to Preview – nodejs-org July 10, 2023 14:33 Inactive
@github-actions
Copy link
Contributor

📦 Next.js Bundle Analysis for nodejs.org

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

@ovflowd ovflowd closed this Jul 10, 2023
@ovflowd ovflowd deleted the feat/generic-time-component branch July 10, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create <Time/> component
3 participants