[ResourceList] refactor to functional component #2843
Conversation
|
👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines. |
|
I also have some ideas about good patterns and further refactorings. There are now some opportunities for separations of concerns around checkboxes, list views etc that could really clean-up this component. I wanted to get a PR in because the change was starting to get big. Excited to talk more about this on Monday. Also, the test coverage was great. Kudos to you all. |
athornburg
force-pushed
the
resource-list-to-functional
branch
2 times, most recently
from
0dafcdb to
cf1f6b0
Compare
athornburg
changed the title
athornburg
changed the title
|
@BPScott thanks for the feedback. Ill have a look. |
ghost
added
the
cla-needed
athornburg
force-pushed
the
resource-list-to-functional
branch
2 times, most recently
from
5961e40 to
cb08355
Compare
ghost
removed
the
cla-needed
|
@BPScott, anymore feedback on this? I'm happy to revise it more. |
There was a problem hiding this comment.
Thanks for your patience, this past fortnight has been pretty hectic.
A few more comments inline with some ideas on how you can simplify some default values. I haven't tested the functional behaviour yet as I'm not familiar ResourceList's inner workings though. @AndrewMusgrave, would you have the time to take over the review of this PR, check if it's working from a functional perspective and get it merged?
|
Hey @BPScott, thanks for getting back to me. Hope you're staying safe. I'll have a look at your comments sometime tomorrow. I can help do some functional testing as well if it helps. |
athornburg
force-pushed
the
resource-list-to-functional
branch
from
1aa5f9d to
bb55c51
Compare
|
@BPScott I think I have incorporated all your feedback. Would be happy to continue revising or start some manual testing with @AndrewMusgrave. Let me know if there is more I can do to help. |
|
Hey @athornburg , sorry about the wait! Hoping to review this in the next couple days 😄 |
There was a problem hiding this comment.
I have a meeting to run to, I will continue reviewing later.
Thanks for doing this @athornburg !
|
Thanks for more feedback @dleroux and @AndrewMusgrave. I'll have a look at this during my lunch break. Happy to help. |
athornburg
force-pushed
the
resource-list-to-functional
branch
from
2c26911 to
600c5d2
Compare
![translation-platform[bot]](https://avatars.githubusercontent.com/in/7649?s=60&v=4){kind=small}
Co-Authored-By: Andrew Musgrave <[email protected]>
athornburg
force-pushed
the
resource-list-to-functional
branch
from
86f1747 to
2128b64
Compare
|
@dleroux When you have a chance it would be great if you can take a look at the sticky header stuff and give a review 😄 |
There was a problem hiding this comment.
@athornburg Thank you so much for doing this an following through! Amazing 🥇 🎉
I top-hatted and works perfectly. 🚢
|
Awesome! Thank you all for the feedback and working with me on this. I had a lot of fun! Hope you all are staying safe and healthy. Let me know if I can help in any other way. |
|
I think CI is gonna get stuck since this is an outside contribution. Let me see if there's a way around it, and if not one of us will merge it for you tomorrow morning |
|
Yep, CI is stuck because we only run certain jobs for Travis branch builds, but since this is a PR with no local branch it only ran the PR build. It's nbd, I'm gonna merge this and make an issue to figure out external CI stuff |
|
🎉 Thanks for your contribution to Polaris React! |
|
Just seeing this now and have to say amazing everyone! Thanks so much @athornburg! 🎉 |
|
Thank you so much @athornburg |
|
Fantastic work! Big thanks to @athornburg for persevering with us and to the Polaris folks for helping get this over the line |
|
No problem! I had a lot of fun working with you all on this. Thanks for all the help, feedback and time. |
WHY are these changes introduced?
In order to make the render item function infer the item's type we refactored to a functional component and made ResourceList generic.
Fixes #543
WHAT is this pull request doing?
More info in the discussion here.
ResourceList will also be a functional component as part of #1995
+ use the 18n hook for internationalization
+ made item generic
+ made the item render functions use the items type
How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
ResourceList is in the storybook already.
🎩 checklist
README.mdwith documentation changes