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.

Sign up for GitHub

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

Jump to bottom

chore(tile): refactor to functional component #9721

Merged
merged 31 commits into from
Oct 13, 2021
Merged

chore(tile): refactor to functional component #9721

merged 31 commits into from
Oct 13, 2021

Conversation

sstrubberg
Copy link
Member

@sstrubberg sstrubberg commented Sep 22, 2021 •
edited
Loading

REF #9712

Refactored most of the Tile component to make functional components using React.forwardRef to ensure refs were being forwarded appropriately.

  • Tile
  • ClickableTile
  • ExpandableTile
  • TileAboveTheFoldContent
  • TileBelowTheFoldContent

Notes

  • I outright gutted the getDerivedStateFromProps from ClickableTile. Everything seems still be working without it and the guidance from React suggests that you rarely need it.

Testing / Reviewing

  1. Pull down the PR locally.

  2. Run yarn start:v11 from packages/react

  3. Ensure that the following stories for Tile are all working as expected:

  • Default
  • ClickableTile
  • Multi Select
  • Radio

Note: I have not refactored ExpandableTile in this PR.

@netlify
Copy link

netlify bot commented Sep 22, 2021 •
edited
Loading

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

🔨 Explore the source changes: 51b9b78

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

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

@netlify
Copy link

netlify bot commented Sep 22, 2021 •
edited
Loading

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

🔨 Explore the source changes: 51b9b78

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

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

@netlify
Copy link

netlify bot commented Sep 22, 2021 •
edited
Loading

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 51b9b78

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

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

@sstrubberg sstrubberg marked this pull request as ready for review September 28, 2021 15:31
@sstrubberg sstrubberg requested a review from a team as a code owner September 28, 2021 15:31
@sstrubberg sstrubberg requested review from tay1orjones, jnm2377 and joshblack and removed request for tay1orjones September 28, 2021 15:31
joshblack
joshblack suggested changes Sep 28, 2021
View reviewed changes
packages/react/.storybook/preview.js Outdated Show resolved Hide resolved
packages/react/src/components/Tile/next/Tile.js Outdated Show resolved Hide resolved
packages/react/src/components/Tile/next/Tile.js Outdated Show resolved Hide resolved
packages/react/src/components/Tile/next/Tile.js Outdated Show resolved Hide resolved
packages/react/src/components/Tile/next/Tile.js Outdated Show resolved Hide resolved
packages/react/src/components/Tile/next/Tile.js Outdated Show resolved Hide resolved
packages/react/src/components/Tile/next/Tile.js Outdated Show resolved Hide resolved
sstrubberg and others added 8 commits September 28, 2021 10:42
@joshblack
Copy link
Contributor

Seems like CI errors are due to a syntax issue on line 119 with {...rest} so should be good to go after that 👀

image

@sstrubberg
Copy link
Member Author

derp

@jnm2377
Copy link
Contributor

jnm2377 commented Oct 5, 2021

Tested components and they're working as expected. Just had those notes on the deprecation stuff.

@sstrubberg
Copy link
Member Author

Tested components and they're working as expected. Just had those notes on the deprecation stuff.

thx! i'll get those warnings in there next :)

jnm2377
jnm2377 reviewed Oct 13, 2021
View reviewed changes
packages/react/package.json
@@ -34,6 +34,7 @@
"postinstall": "carbon-telemetry collect --install",
"prepublish": "yarn build",
"start": "yarn storybook",
"start:v11": "CARBON_ENABLE_V11_RELEASE=true yarn storybook",
Copy link
Contributor

Choose a reason for hiding this comment

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

NOICEEEE!!! 🔥

@kodiakhq kodiakhq bot merged commit 83f2658 into carbon-design-system:main Oct 13, 2021
@sstrubberg sstrubberg deleted the tile-functional branch October 13, 2021 18:28
@tay1orjones tay1orjones mentioned this pull request Oct 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshblack joshblack joshblack approved these changes

@jnm2377 jnm2377 jnm2377 approved these changes

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sstrubberg @joshblack @jnm2377