Select Deprecation edits #2434
Select Deprecation edits #2434
Conversation
- Updates TODO Comment to highlight more obviously in most IDEs
- We have a convention of preferring named exports, and that's what we export in index.tsx
brandonlenz
requested review from
gidjin,
haworku,
ahobson and
SirenaBorracha
as code owners
| const { queryByTestId } = render( | ||
| <Select id="input-type-text" name="input-type-text"> | ||
| <Dropdown id="input-type-text" name="input-type-text"> |
There was a problem hiding this comment.
This test should still be for the Dropdown component. Might've accidentally gotten updated by IDE in #2415
| expect(deprecationWarning).toHaveBeenCalledTimes(1) | ||
| expect(deprecationWarning).toHaveBeenCalledWith( | ||
| 'Dropdown is deprecated and will be removed in the future. Please use the Select component instead.' | ||
| ) |
There was a problem hiding this comment.
Not really necessary, since deprecationWarning and withDeprecationWarning have tests, but it was peace of mind for me making sure that my suggestions actually work because this was just a theory until confirming it 😉
There was a problem hiding this comment.
bah i tried adding this but couldn't get it to work
| export const Dropdown = withDeprecationWarning( | ||
| Select, | ||
| 'Dropdown is deprecated and will be removed in the future. Please use the Select component instead.' | ||
| ) |
There was a problem hiding this comment.
@werdnanoslen this was the main thing I wanted to suggest. withDeprecationWarning allows you to wrap any component, and it just returns a new component with the name you give it, which renders with a useEffect that calls deprecationWarning on initial render, and also renders the component you pass in.
So no need to define and pass around props, it basically inherits all of that from Select. Dropdown is now just Select with the old name, that also renders a warning.
🪄 Magic 🪄
There was a problem hiding this comment.
ahh got it, haven't used a pattern like this (intentionally) before, but that makes sense!
| @@ -48,5 +48,3 @@ export const Select = ({ | |||
| </select> | |||
| ) | |||
| } | |||
|
|
|||
| export default Select | |||
There was a problem hiding this comment.
I noticed this leading to some new(?) eslint warnings. Since the default export had no usages, and we seem to prefer the named export as a convention, this can be removed 😄
Summary
Batched PR feedback/suggestions for #2415
How To Test
Unit tests, storybook
Screenshots (optional)
Dropdown still works, and displays deprecation warning:
Select of course also works, does not display deprecation warning:
