Details

Reviewers

tomek
atul
• benschac

Commits

rCOMM2986754c24d3: [web] Convert `Modal` into functional component

Summary

Convert exiting Modal component from class into functional component to simplify the logic and make future changes easier. No logic changes has been made.

Test Plan

All existing modals should work as before

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• jacek created this revision.Apr 8 2022, 4:24 AM

Herald added subscribers: • abosh, • benschac, • karol and 4 others. · View Herald TranscriptApr 8 2022, 4:24 AM

Harbormaster completed remote builds in B8019: Diff 11224.Apr 8 2022, 4:30 AM

• jacek requested review of this revision.Apr 8 2022, 4:30 AM

• jacek added a child revision: D3669: [web] Redesign `Modal` to match Figma.Apr 8 2022, 6:01 AM

• jacek added a parent revision: D3666: [web] Center modals vertically.

• jacek added reviewers: tomek, atul, • benschac.Apr 8 2022, 6:06 AM

Looks good!

web/modals/modal.react.js
18 ↗	(On Diff #11224)	Setting the default when destructuring looks a lot cleaner than the old `defaultProps` approach.
32 ↗	(On Diff #11224)	Outside of the scope of this diff, but might be good to add a comment noting that `keyCode` 27 corresponds to the `ESC` key? I had a hunch, but still had to google to double check.

This revision is now accepted and ready to land.Apr 8 2022, 10:38 AM

• jacek added inline comments.Apr 10 2022, 11:40 PM

web/modals/modal.react.js
32 ↗	(On Diff #11224)	Good idea. I was thinking about adding the comment (as I also had to use https://keycode.info/ to check), but I forgot about it.