refactor(react): update ContentSwitcher and Switch to hooks #3302
Conversation
…refactor/update-content-switch-to-hooks
…refactor/update-content-switch-to-hooks
ghost
requested review from
jnm2377 and
lovemecomputer
and removed request for
a team
|
Deploy preview for the-carbon-components ready! Built with commit 2276790 https://deploy-preview-3302--the-carbon-components.netlify.com |
|
Deploy preview for carbon-components-react ready! Built with commit 2276790 https://deploy-preview-3302--carbon-components-react.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit 2276790 |
|
Deploy preview for the-carbon-components ready! Built with commit 6667a27 https://deploy-preview-3302--the-carbon-components.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit 6667a27 |
|
Deploy preview for carbon-components-react ready! Built with commit 6667a27 https://deploy-preview-3302--carbon-components-react.netlify.com |
| import { settings } from 'carbon-components'; | ||
| import cx from 'classnames'; |
There was a problem hiding this comment.
why abbreviate to cx? i typically prefer more readable/explicit names
There was a problem hiding this comment.
cx comes from the "class set" convention from earlier versions of React 👍 Can definitely map it over now if that convention is outdated
There was a problem hiding this comment.
in this case i usually see it imported as classnames around the codebase
There was a problem hiding this comment.
@lovemecomputer it's definitely a mix across the codebase depending on the author, we can totally consolidate though. cx to me is helpful because it's shorter and has been a convention since early versions of React but I understand if that is no longer a relevant reference.
|
Note to reviewers: we have a slight bug with the controlled prop where if it changes we are accidentally focusing the switch at the new index 🤦♂ will update accordingly! |
| if (controlledSelectedIndex !== prevControlledIndex) { | ||
| setSelectedIndex(controlledSelectedIndex); | ||
| setPrevControlledIndex(controlledSelectedIndex); | ||
| setShouldFocus(false); |
There was a problem hiding this comment.
Does the rendering code after this line stop focusing or not?
There was a problem hiding this comment.
Here's a quick isolated demo that I think demonstrates what's going on here: https://codesandbox.io/s/jolly-antonelli-uevex
Associated demo:
All this is to say that when we enqueue this state transition the effect is never re-run with the truth-y value in the effect. Instead, React runs the effect with the false-y value.
There was a problem hiding this comment.
When you view the storybook, you'll notice that on first render we have no focus inside the ContentSwitcher alongside when selectedIndex is changed through the knob 👍
There was a problem hiding this comment.
Cool, makes sense now, thank you for clarifying.
packages/react/src/components/ContentSwitcher/__tests__/ContentSwitcher-test.js
Outdated
Show resolved
Hide resolved
Update
ContentSwitcherandSwitchcomponents to use hooks. This includes some general clean-ups as these codepaths haven't been updated in a while, namely around the test suite and props. This update also consolidatesContentSwitcherandSwitchunder the same folder, making sure to add in aliases to the new paths.Changelog
New
Changed
ContentSwitcherhas been updated to use hooksSwitchhas been updated to reflect changes inContentSwitcher.nameprop onSwitchas we can now useindexinsteaddeprecateprop type helperContentSwitcherandSwitchRemoved