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(TimePicker): class -> functional component #9921

Merged
merged 27 commits into from
Oct 30, 2021

Conversation

sstrubberg
Copy link
Member

@sstrubberg sstrubberg commented Oct 21, 2021

REF #9712

Refactored TimePicker into a functional component with RTL tests.

  • Removed instances of evt.persist(); because it is no longer needed as of React 17. However, this causes Storybook to generate console error messages when using Actions, so I removed them.

Testing / Reviewing

  1. Pull down locally
  2. Go to packages/react
  3. Run yarn start:v11 to run old storybook with v11 components.
  4. Ensure tests for Next versions of TimePicker go green.

@netlify
Copy link

netlify bot commented Oct 21, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 01e8eff

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/617c9f9aca0ffe00086935c1

😎 Browse the preview: https://deploy-preview-9921--carbon-react-next.netlify.app

@sstrubberg sstrubberg marked this pull request as ready for review October 21, 2021 20:57
@sstrubberg sstrubberg requested a review from a team as a code owner October 21, 2021 20:57
@netlify
Copy link

netlify bot commented Oct 21, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 01e8eff

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/617c9f9a9796080008110a88

😎 Browse the preview: https://deploy-preview-9921--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Oct 21, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 01e8eff

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/617c9f9a7c2b1500079172e0

😎 Browse the preview: https://deploy-preview-9921--carbon-elements.netlify.app

Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Looking great! Just left a couple of style nits

@sstrubberg sstrubberg requested a review from joshblack October 25, 2021 14:56
@sstrubberg sstrubberg requested a review from joshblack October 25, 2021 19:35
Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

The storybook actions throw this error:

Warning: This synthetic event is reused for performance reasons. If you're seeing this, you're accessing the property buttons on a released/nullified synthetic event. This is set to null. If you must keep the original synthetic event around, use event.persist(). See https://fb.me/react-event-pooling for more information.

Will users accessing the onChange, onClick, and onBlur get this same error message if they're using React v16?

The React v17 blog post has a brief note that event.persist() is still available on the React event object, it just doesn't do anything in React v17.

Should we continue to use event.persist() and wait to remove it until (if?) we remove support for React v16?

@joshblack
Copy link
Contributor

@tay1orjones I think for v11 we're going to move to v17 React as a baseline so it should be okay. What do you think?

@tay1orjones
Copy link
Member

@joshblack very exiting! I didn't know that was on your mind for v11. Totally approve.

@sstrubberg sstrubberg merged commit 37a0536 into carbon-design-system:main Oct 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants