Page MenuHomePhabricator

[web] [refactor] add clearModal context to modal component
AbandonedPublic

Authored by benschac on Feb 17 2022, 7:28 AM.
Tags
None
Referenced Files
F3249137: D3241.diff
Fri, Nov 15, 12:27 PM
Unknown Object (File)
Fri, Nov 8, 2:05 AM
Unknown Object (File)
Sat, Nov 2, 6:18 AM
Unknown Object (File)
Oct 7 2024, 4:21 AM
Unknown Object (File)
Oct 7 2024, 4:21 AM
Unknown Object (File)
Oct 7 2024, 4:19 AM
Unknown Object (File)
Sep 16 2024, 11:56 PM
Unknown Object (File)
Sep 15 2024, 4:03 AM

Details

Reviewers
atul
ashoat
Summary

Add modal context to the Modal so we don't have to prop drill.

Test Plan

Modal should open and close as expected.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

web/modals/modal.react.js
91

I couldn't think of a better way to not re-write the entire component as a function component.

Curious if there's a better way.

ashoat requested changes to this revision.Feb 21 2022, 10:14 PM

Please make the dependency graph show up by adding "Depends on: Dwhatever" to the diff summary, and then re-request review.

web/modals/modal.react.js
89

My understanding is that you should avoid defining React components using this notation. I'm pretty sure this is something I've given you feedback on before, so please keep it in mind going forward.

My memory is that by defining your React component using this notation, you skip specifying a name or displayName.

I've noticed this syntax in some open source libraries... not sure if something has changed and my info is outdated. The last time I mentioned this feedback to you, I linked a StackOverflow thread (or maybe GitHub comment?) – if you want to dig up the context, you can search for that. Willing to be convinced if you want to argue for this, but would encourage you to not spend too much time on it.

At a higher level, it's critical that you're able to absorb and integrate feedback in diff review – otherwise you will keep making the same mistakes, and I will continue having to spend a large amount of time writing comments on your diffs. I'm always more concerned when I have to repeat the same comment twice, then I ever am for a first-time comment.

This revision now requires changes to proceed.Feb 21 2022, 10:14 PM
web/modals/modal.react.js
89

You're right. This was an oversight. I was going to add a React.memo here but didn't and kept the const ComponentName