Convert MultiSelect and SingleSelect to functional components #2370
Conversation
…ner function since that's where sharedProps is applied
🦋 Changeset detectedLatest commit: 32b4f84 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: +1 B (0%) Total Size: 101 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-hhnmdfvape.chromatic.com/ Chromatic results:
|
beaesguerra
force-pushed
the
dropdown-fc
branch
from
cad7bd0
to
32b4f84
Compare
| type State = Readonly<{ | ||
| /** | ||
| * Whether or not the dropdown is open. | ||
| */ | ||
| open: boolean; | ||
| /** | ||
| * The text input to filter the items by their label. Defaults to an empty | ||
| * string. | ||
| */ | ||
| searchText: string; | ||
| /** | ||
| * The selected values that are set when the dropdown is opened. We use | ||
| * this to move the selected items to the top when the dropdown is | ||
| * re-opened. | ||
| */ | ||
| lastSelectedValues: Array<string>; | ||
| /** | ||
| * The object containing the custom labels used inside this component. | ||
| */ | ||
| labels: Labels; | ||
| /** | ||
| * The DOM reference to the opener element. This is mainly used to set focus | ||
| * to this element, and also to pass the reference to Popper.js. | ||
| */ | ||
| openerElement?: HTMLElement; | ||
| }>; |
There was a problem hiding this comment.
open, searchText, lastSelectedValues and openerElement have useState equivalents
wonder-blocks/packages/wonder-blocks-dropdown/src/components/multi-select.tsx
Lines 223 to 238 in e02ada1
labels doesn't need to be in state and combines the default labels and prop labels:
wonder-blocks/packages/wonder-blocks-dropdown/src/components/multi-select.tsx
Lines 220 to 221 in e02ada1
| export default class MultiSelect extends React.Component<Props, State> { | ||
| labels: Labels; | ||
|
|
||
| static defaultProps: DefaultProps = { |
There was a problem hiding this comment.
Default props are set when we deconstruct props in the functional component
wonder-blocks/packages/wonder-blocks-dropdown/src/components/multi-select.tsx
Lines 194 to 218 in e02ada1
| labels: {...defaultLabels, ...props.labels}, | ||
| }; | ||
| // merge custom labels with the default ones | ||
| this.labels = {...defaultLabels, ...props.labels}; |
There was a problem hiding this comment.
labels on the class wasn't being used so it can be removed. We combine default labels and prop labels:
wonder-blocks/packages/wonder-blocks-dropdown/src/components/multi-select.tsx
Lines 220 to 221 in e02ada1
| // open should always be false if select is disabled | ||
| open: props.disabled | ||
| ? false | ||
| : typeof props.opened === "boolean" | ||
| ? props.opened | ||
| : state.open, |
There was a problem hiding this comment.
The logic for getDerivedStateFromProps is translated to a useEffect:
- If
disabledprop is set totrue, we update the localopenstate tofalse. - If the
openedprop changes, we update the localopenstate to the value of the prop - Otherwise, don't change local
openstate
wonder-blocks/packages/wonder-blocks-dropdown/src/components/multi-select.tsx
Lines 240 to 248 in 32b4f84
| componentDidUpdate(prevProps: Props) { | ||
| if (this.props.labels !== prevProps.labels) { | ||
| // eslint-disable-next-line react/no-did-update-set-state | ||
| this.setState({ | ||
| labels: {...this.state.labels, ...this.props.labels}, | ||
| }); |
There was a problem hiding this comment.
Labels will stay up to date since it will combine the default labels and the prop labels
wonder-blocks/packages/wonder-blocks-dropdown/src/components/multi-select.tsx
Lines 220 to 221 in 32b4f84
| light, | ||
| opener, | ||
| testId, | ||
| // the following props are being included here to avoid |
There was a problem hiding this comment.
Since we deconstruct the props at the start of the function, we don't need to again in the renderOpener function.
wonder-blocks/packages/wonder-blocks-dropdown/src/components/multi-select.tsx
Lines 194 to 218 in 32b4f84
The deconstruction previously is almost the same. The FC will also extract disabled, error, children, and dropdownId so they won't be included in sharedProps when it is spread. The only one that matters to the SelectOpener is error, which is why we also explicitly set the error prop: https://github.com/Khan/wonder-blocks/pull/2370/files#diff-f7d0f4de4f6447c1f878dd6c51e6646adae276658cf9d7f29fcea7f630b17430R484
(changes for the disabled doesn't matter since we were already explicitly setting it https://github.com/Khan/wonder-blocks/pull/2370/files#diff-f7d0f4de4f6447c1f878dd6c51e6646adae276658cf9d7f29fcea7f630b17430R485)
| @@ -48,16 +48,16 @@ type DefaultProps = Readonly<{ | |||
| * Whether this dropdown should be left-aligned or right-aligned with the | |||
| * opener component. Defaults to left-aligned. | |||
| */ | |||
| alignment: "left" | "right"; | |||
There was a problem hiding this comment.
Most of the changes in SingleSelect are similar to the ones annotated in MultiSelect. Will highlight specific changes related to SingleSelect
| constructor(props: Props) { | ||
| super(props); | ||
|
|
||
| this.selectedIndex = 0; |
There was a problem hiding this comment.
We use a ref for this now:
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (af050b1) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2370"Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2370 |
Summary:
Convert MultiSelect and SingleSelect to functional components to support the validation work to support LabeledField.
I was trying to add in the validation logic without refactoring too much since we might replace these components with Combobox. It was more complicated to account for different cases so I am refactoring it first to make it easier!
Issue: WB-1782
Test plan:
(would appreciate help with extra testing in case I missed something. These components were a bit more complicated to convert to FC!)