-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Post v4 cleanup work #1995
Comments
Audits for hooks conversionTo help identify where we can focus for quick wins, here's an audit of the difficulty of migrating components (half done, I still need to finish off several files). Files using withAppProvider are a priority, then easy other class-based components Difficulty is a 1 to 5 scale, with 1 being easiest, and 5 being hardest.
Blasting out 5 or so 1 or 2 level complexity components in a single PR is fine as changes should be reasonably straightforward to understand (and pretty minimal if you supress whitespace in the diff), but beyond that review gets harder so one or two components per PR. All components that extend from React.Component or React.PureComponent (
|
Filename | Difficulty | Notes |
---|---|---|
AppProvider/AppProvider.tsx | 3 | Has react lint warnings mentioning 'eslint-disable-next-line react/' |
BulkActions/BulkActions.tsx | ||
ColorPicker/ColorPicker.tsx | ||
ColorPicker/components/ AlphaPicker/AlphaPicker.tsx | ||
ColorPicker/components/ HuePicker/HuePicker.tsx | ||
ColorPicker/components/ Slidable/Slidable.tsx | ||
DataTable/DataTable.tsx | 5 | |
DropZone/DropZone.tsx | ||
EventListener/EventListener.tsx | ||
Filters/components/ ConnectedFilterControl/ConnectedFilterControl.tsx | ||
Filters/Filters.tsx | 5 | |
Frame/Frame.tsx | ||
Popover/components/ PopoverOverlay/PopoverOverlay.tsx | ||
PositionedOverlay/PositionedOverlay.tsx | ||
RangeSlider/components/ DualThumb/DualThumb.tsx | ||
ResourceItem/ResourceItem.tsx | 5 | Uses "globalIdGeneratorFactory" when using useUniqueId() would be a preferable |
Scrollable/Scrollable.tsx | ||
Sticky/Sticky.tsx | 3 | |
Tabs/components/ Item/Item.tsx | ||
Tabs/Tabs.tsx | 3.5 |
can I migrate |
Absolutely!! I've updated the |
Updated the tables to remove the completed items |
can I migrate |
Hey @sijad, go for it!! I've updated the table 👍 |
Taking care of the |
Taking care of Collapsible #3606 |
This issue seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. |
Migration to functional components is still worthwhile IMO. |
Hi! We noticed there hasn’t been activity on this issue in a while. After 30 days, it will close automatically. If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed. |
Hi! We noticed there hasn’t been activity on this issue in a while. After 30 days, it will close automatically. If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed. |
Hi! We noticed there hasn’t been activity on this issue in a while. After 30 days, it will close automatically. If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed. |
@BPScott I was going to get around to this 😅 |
This is a meta issue to collate what I'd like to do as tidy-up work in a post-v4 world (in v4.1.0, v4.2.0 and beyond).
I'd like for us to do three things, in vague order of priority. Much of this conversion work has several quick wins for components where a migration to the new style is easy, we should prioritize that easy work, and punt on the fewer harder migrations. That way our first timebox of work shall feel like good progress and make future planning easier (as there are fewer items to deal with overall)
DONE!
Convert all examples to use hooks instead of classes.Shopify is rallying around hooks as the new best practice - we should help ensure people follow that by giving up to date examples.
Searching for "extends React." in README.md files should return no results.
@AndrewMusgrave made a pretty big start on this in https://github.com/Shopify/polaris-react/compare/hookify-examples but I'm not sure how polished that branch is. It might be worth splitting off chunks of that branch into separate PRs (5 or so README files at a time depending on the ease of the comparison) and reviewing them individually so the changes are more digestible.
Convert existing code to use hooks instead of classes
Hooks are the way forward and we should leverage them. We should prioitize components that use withAppProvider (as we want to remove that HoC), and small components where making the change is easy.
@AndrewMusgrave made a start on this in https://github.com/Shopify/polaris-react/compare/fast-hookifies but I am mot sure how polished that branch is. As above, splitting this out into multiple PRs for ease of review would be useful
Done
Migrate our tests fromenzyme
to use@shopify/react-testing
Dedicated issue: #4274
This is a bunch of work with not much value behind it (aside from enzyme being often low-key annoying), this can probably be deferred and deferred until the end of time.
One the up side @vsumner has wrote a codemod in web that will bulk convert a codebase so that'd be worth investigating.
The text was updated successfully, but these errors were encountered: