Page MenuHomePhabricator

[web] Allow passing `Modal` parameters into `SearchModal`
ClosedPublic

Authored by jacek on Apr 8 2022, 5:56 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 5, 4:10 AM
Unknown Object (File)
Tue, Nov 5, 4:10 AM
Unknown Object (File)
Tue, Nov 5, 4:10 AM
Unknown Object (File)
Tue, Nov 5, 4:10 AM
Unknown Object (File)
Tue, Nov 5, 4:10 AM
Unknown Object (File)
Tue, Nov 5, 4:10 AM
Unknown Object (File)
Tue, Nov 5, 4:10 AM
Unknown Object (File)
Tue, Nov 5, 4:04 AM

Details

Summary

Allow setting modal properties when using SearchModal. Will be used in following diffs.

Test Plan

All modals should work as before. Can be tested after next diff in stack.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

I've opened a similar diff D3644, but feel free to land yours

This revision is now accepted and ready to land.Apr 11 2022, 6:50 AM

Allow passing other Modal parameters

jacek retitled this revision from [web] Allow passing `size` parameter into `SearchModal` to [web] Allow passing `Modal` parameters into `SearchModal`.
jacek edited the summary of this revision. (Show Details)
tomek requested changes to this revision.Apr 15 2022, 8:41 AM

Looks really nice!

web/modals/modal.react.js
10–16 ↗(On Diff #11468)

I think we should split this type into two parts: with overridable and non-overridable props. The second category contains only children. Currently we replace it with +children: (searchText: string) => React.Node, but it would be cleaner to not include it in ModalProps - it will make this code more maintainable.

18 ↗(On Diff #11468)

We usually try not to mix default and named exports. This component already had this issue, but we can easily fix that

This revision now requires changes to proceed.Apr 15 2022, 8:41 AM
web/modals/modal.react.js
18 ↗(On Diff #11468)
  1. Please avoid inline export statements, except in files like lib/types that have a ton of exports. Instead, have a single export line at the bottom
  2. We have a lot of places where we export default with no export, but do we have export type. I don't feel strongly, but worth noting that today the standard in the codebase about export default vs. export does not apply to export type

Excluded overridable Modal props into separate type

This revision is now accepted and ready to land.Apr 20 2022, 9:40 AM