Add modal context to the Modal so we don't have to prop drill.
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. |
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. |
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 |