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

docs(date): rfc proposing new date component #4294

Merged
merged 2 commits into from
Nov 9, 2021
Merged

Conversation

edleeks87
Copy link
Contributor

@edleeks87 edleeks87 commented Aug 9, 2021

Proposed behaviour

RFC detailing a proposal for a new implementation for the Date component

Current behaviour

Existing implementation has considerable technical debt and issues and pending features

Checklist

  • Commits follow our style guide

Additional context

POC (very rough implementation):
https://codesandbox.io/s/poc-of-rfc-proposal-ctdfw?file=/src/index.js

Testing instructions

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 9, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 59353fb:

Sandbox Source
carbon-quickstart Configuration
carbon-quickstart PR
POC of RFC proposal PR

@cypress
Copy link

cypress bot commented Aug 9, 2021



Test summary

1242 0 3 0Flakiness 0


Run details

Project carbon
Status Passed
Commit a55316c5a9 ℹ️
Started Aug 9, 2021 2:56 PM
Ended Aug 9, 2021 3:03 PM
Duration 06:18 💡
OS Linux Debian - 10.8
Browser Chrome 90

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Copy link
Contributor

@samtjo samtjo left a comment

Choose a reason for hiding this comment

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

The approach looks good to me in general, mostly minor comments/changes from me

rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
Copy link

@jamime jamime left a comment

Choose a reason for hiding this comment

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

  • Update RFC to use composition for date picker e.g. <Date picker={<Picker />} />

@cypress
Copy link

cypress bot commented Sep 22, 2021



Test summary

1177 0 3 0Flakiness 0


Run details

Project carbon
Status Passed
Commit ff1c00f
Started Nov 9, 2021 9:59 AM
Ended Nov 9, 2021 10:03 AM
Duration 03:57 💡
OS Linux Debian - 10.9
Browser Chrome 91

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
@mkrds
Copy link
Contributor

mkrds commented Sep 29, 2021

Looks good to me, I'm just not sure about the renderPicker approach, if there are use cases for that than I'm ok with that

Copy link

@jamime jamime left a comment

Choose a reason for hiding this comment

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

Meeting Notes 30th Sept

Attendees: @edleeks87 @mkrds

  • Change renderPicker to be pickerProps and pass props to react-day-picker
  • Link to react-day-picker documentation
  • Add notes on captionElement. We will add additional prop e.g. displayInputCaption which will add our Select components for switching the month/year.

@samtjo samtjo added RFC Request for Comments and removed RFC Request for Comments labels Oct 29, 2021
rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
rfcs/text/refactoring-date-component.md Outdated Show resolved Hide resolved
jamime
jamime previously approved these changes Nov 2, 2021
jamime
jamime previously approved these changes Nov 3, 2021
samtjo
samtjo previously approved these changes Nov 3, 2021
chrisbarber86
chrisbarber86 previously approved these changes Nov 4, 2021
@edleeks87
Copy link
Contributor Author

edleeks87 commented Nov 4, 2021

PR has moved into final comments stage, this PR will be merged in 3 days time if no further changes are required

@edleeks87 edleeks87 marked this pull request as ready for review November 9, 2021 11:10
@edleeks87 edleeks87 requested a review from a team as a code owner November 9, 2021 11:10
@edleeks87 edleeks87 merged commit 6b577ef into master Nov 9, 2021
@edleeks87 edleeks87 deleted the FE-2543-date-rfc branch November 9, 2021 11:17
@carbonci
Copy link
Collaborator

🎉 This PR is included in version 97.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@quinnturner
Copy link

quinnturner commented Dec 9, 2021

Regarding alternatives to moment, I'd like to add another point to the discussion; migration workload for downstream projects.

Since carbon-react is the last project that uses moment within our bundle, we will probably end up going with whatever carbon-react chooses to minimize our bundle size. dayjs generally has a smoother transition from moment than date-fns because dayjs is closer to being API-compatible than date-fns (after using a few dayjs plugins). Therefore, it may be less work from each dependent project on carbon-react, that wishes to use the same date lib, to upgrade as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

9 participants