Paths

Table of Contentst

Differential D3670

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

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

Subscribers

Details

Reviewers

tomek
• benschac
atul

Commits

rCOMMd0de89e4792c: [web] Allow passing `Modal` parameters into `SearchModal`

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

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• jacek created this revision.Apr 8 2022, 5:56 AM

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

• jacek added a child revision: D3671: [web] Adjust `MembersModal` size to match designs.Apr 8 2022, 5:59 AM

• jacek added a child revision: D3672: [web] Adjust `SubchannelsModal` size to match designs.

Harbormaster completed remote builds in B8022: Diff 11227.Apr 8 2022, 6:02 AM

• jacek requested review of this revision.Apr 8 2022, 6:02 AM

• jacek added a reviewer: tomek.Apr 8 2022, 6:07 AM

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 requested review of this revision.Apr 14 2022, 3:16 AM

• 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)

Harbormaster completed remote builds in B8197: Diff 11468.Apr 14 2022, 3:22 AM

• jacek added reviewers: • benschac, atul.Apr 15 2022, 3:07 AM

• jacek added a child revision: D3739: [web] Introduce `AddMembersItem` component.Apr 15 2022, 4:04 AM

Looks really nice!

web/modals/modal.react.js
10–16	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	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

ashoat added inline comments.Apr 18 2022, 12:24 PM

web/modals/modal.react.js
18	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 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

Harbormaster completed remote builds in B8325: Diff 11630.Apr 19 2022, 11:38 AM

tomek accepted this revision.Apr 20 2022, 9:40 AM

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

rebase

Harbormaster completed remote builds in B8521: Diff 11880.Apr 25 2022, 10:42 AM

rebase

Harbormaster completed remote builds in B8556: Diff 11925.Apr 26 2022, 8:49 AM

Closed by commit rCOMMd0de89e4792c: [web] Allow passing `Modal` parameters into `SearchModal` (authored by • jacek). · Explain WhyApr 26 2022, 9:09 AM

This revision was automatically updated to reflect the committed changes.

• jacek added a commit: rCOMMd0de89e4792c: [web] Allow passing `Modal` parameters into `SearchModal`.

Revision Contents
Changeset List

Path

Size

web/

modals/

4 lines

search-modal.react.js

11 lines

Diff 11468

web/modals/modal.react.js

Loading...

web/modals/search-modal.react.js

Loading...