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
MBS-11514 / MBS-11515 / MBS-11516 / MBS-11517: Use React.useContext in more places rather than passing $c around #1850
Conversation
75558a2
to
6ae8f80
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proofreading the code is blazing fast when hiding space changes indeed. Quickly tested it too.
root/collection/CollectionIndex.js
Outdated
@@ -161,8 +158,9 @@ const listPicker = ( | |||
|
|||
const CollectionIndex = (props: Props): | |||
React.Element<typeof CollectionLayout> => { | |||
const $c = React.useContext(CatalystContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better not to do this for top-level components, since it's always passed as a prop in that case: https://github.com/metabrainz/musicbrainz-server/blob/b036579/root/server/response.js#L100
So it's actually simpler to just use the prop here. I'm not sure if this is the only example in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rechecked, I think this was the only one. Amended.
9bc055d
to
04d3827
Compare
There's also no need to pass the different bits of stash separately. It's much more clean to just pass entity and assign the rest from context on the component.
Implements MBS-11514 / MBS-11515 / MBS-11516 / MBS-11517
Some big chunks (like Layout) left for a second PR to avoid making this huge. Reports bits done in #1787 since that already refactors the whole thing.
This shaves some lines, but more importantly it makes it easier to understand and follow and avoids passing $c down several components just to reach the one N steps down the line.
I'd suggest reviewing with the whitespace ignore on, since most changes other than just the $c bit itself are to indent some components because of adding return () to them.
Tested by navigating around locally, but not extensively - plus Flow to ensure I didn't keep passing $c where I shouldn't anymore.