Page MenuHomePhabricator
Differential D10521

[web] update members modal to use the new tabs component
ClosedPublic
Actions

Authored by ginsu on Jan 4 2024, 1:13 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 13, 4:27 PM
Unknown Object (File)
Mon, Nov 11, 9:29 AM
Unknown Object (File)
Wed, Nov 6, 1:35 PM
Unknown Object (File)
Thu, Oct 31, 4:22 AM
Unknown Object (File)
Thu, Oct 31, 4:22 AM
Unknown Object (File)
Thu, Oct 31, 4:22 AM
Unknown Object (File)
Thu, Oct 31, 4:22 AM
Unknown Object (File)
Thu, Oct 31, 4:21 AM
View All 57 Files
Subscribers
ashoat
tomek

Details

Reviewers
atul
inka
Commits
rCOMM37f34c5c2f8c: [web] update members modal to use the new tabs component
Summary

This diff updates the members modal to use the new tab components. Since we decoupled the tabs from the tab content with this new tab component, the members modal now needs to be responsible for rendering the correct tab content.

Depends on D10520

Test Plan

Please see the demo video to confirm that there are no regressions

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu created this revision.Jan 4 2024, 1:13 AM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptJan 4 2024, 1:13 AM
ginsu edited the summary of this revision. (Show Details)Jan 4 2024, 1:16 AM
ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, inka.
ginsu added a parent revision: D10520: [web] update thread settings media gallery modal to use new tabs component.
ginsu added a child revision: D10522: [web] update thread settings modal to use the new tabs component.Jan 4 2024, 1:25 AM
Harbormaster completed remote builds in B25473: Diff 35187.Jan 4 2024, 1:31 AM
ginsu requested review of this revision.Jan 4 2024, 1:31 AM
atul accepted this revision.Jan 4 2024, 12:57 PM

Looks good

web/modals/threads/members/members-modal.react.js
26–35 ↗(On Diff #35187)

Same feedback as rest of the stack, but the id and headers seem redundant. Let me know if there's something I'm missing?

115–121 ↗(On Diff #35187)

Wouldn't hurt to sneak a React.useMemo here..

This revision is now accepted and ready to land.Jan 4 2024, 12:57 PM
ginsu added inline comments.Jan 5 2024, 12:30 PM
web/modals/threads/members/members-modal.react.js
26–35 ↗(On Diff #35187)

I made the header field a type of React.Node. I did this so that if we ever wanted to add other elements to the header component we could just pass in a react component as the value of this field. I could imagine us wanting to put icons in the future or doing other things and I thought this would be the best way to make the tabs component as flexible/future proof as possible

ginsu updated this revision to Diff 35305.Jan 5 2024, 12:32 PM

address comments + rebase before landing

Harbormaster completed remote builds in B25550: Diff 35305.Jan 5 2024, 12:52 PM
Closed by commit rCOMM37f34c5c2f8c: [web] update members modal to use the new tabs component (authored by ginsu). · Explain WhyJan 5 2024, 1:39 PM
This revision was automatically updated to reflect the committed changes.
ginsu added a commit: rCOMM37f34c5c2f8c: [web] update members modal to use the new tabs component.

Revision Contents
Changeset List

PathSize
web/
modals/
threads/
members/
70 lines

Diff 35305

View Options

web/modals/threads/members/members-modal.react.js

Loading...