Page MenuHomePhabricator
Differential D10522

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

Authored by ginsu on Jan 4 2024, 1:18 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 31, 4:21 AM
Unknown Object (File)
Sat, Oct 19, 9:50 AM
Unknown Object (File)
Sat, Oct 19, 9:50 AM
Unknown Object (File)
Sat, Oct 19, 9:50 AM
Unknown Object (File)
Sat, Oct 19, 9:50 AM
Unknown Object (File)
Sat, Oct 19, 9:50 AM
Unknown Object (File)
Sep 14 2024, 9:18 PM
Unknown Object (File)
Sep 14 2024, 9:18 PM
View All 52 Files
Subscribers
ashoat
tomek

Details

Reviewers
atul
inka
Commits
rCOMMc3f4b7b7689a: [web] update thread settings modal to use the new tabs component
Summary

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

Depends on D10521

Test Plan

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

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu created this revision.Jan 4 2024, 1:18 AM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptJan 4 2024, 1:18 AM
ginsu edited the summary of this revision. (Show Details)Jan 4 2024, 1:25 AM
ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, inka.
ginsu added a parent revision: D10521: [web] update members modal to use the new tabs component.
ginsu attached a referenced file: F1026368: Screen Recording 2024-01-04 at 4.22.39 AM.mov. (Show Details)
ginsu added inline comments.Jan 4 2024, 1:32 AM
web/modals/threads/settings/thread-settings-modal.react.js
140–176 ↗(On Diff #35188)

Refactored this so instead of pushing Tab.Items based on certain conditions, we now instead push into tabsData and let our new tabs component figure out which tabs we should show.

Harbormaster completed remote builds in B25474: Diff 35188.Jan 4 2024, 1:35 AM
ginsu requested review of this revision.Jan 4 2024, 1:35 AM
ginsu added a child revision: D10523: [web] update chat tabs to use the new tabs component.Jan 4 2024, 1:38 AM
atul accepted this revision.Jan 4 2024, 1:20 PM

Looks good

web/modals/threads/settings/thread-settings-modal.react.js
258–268 ↗(On Diff #35188)

Would be good to memoize this

This revision is now accepted and ready to land.Jan 4 2024, 1:20 PM
atul added a comment.Jan 4 2024, 1:21 PM

Looks like there might be a regression with the info icon not appearing correctly in the Delete tab:

8af95f.png (482×706 px, 46 KB)

It looks like this in my local environment:

968f99.png (480×824 px, 36 KB)

Can you please check this really quick before landing.

ginsu added inline comments.Jan 5 2024, 12:50 PM
web/modals/threads/settings/thread-settings-modal.react.js
258–268 ↗(On Diff #35188)

spoke offline with @atul but this entire component is wrapped in a React.memo

ginsu updated this revision to Diff 35307.Jan 5 2024, 12:51 PM

address comments

ginsu added a comment.Jan 5 2024, 12:52 PM

Looks like there might be a regression with the info icon not appearing correctly in the Delete tab:

Updated the diff to fix this

Screenshot 2024-01-05 at 3.52.22 PM.png (2×3 px, 896 KB)

Harbormaster completed remote builds in B25552: Diff 35307.Jan 5 2024, 1:09 PM
Closed by commit rCOMMc3f4b7b7689a: [web] update thread settings 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: rCOMMc3f4b7b7689a: [web] update thread settings modal to use the new tabs component.

Revision Contents
Changeset List

PathSize
web/
modals/
threads/
settings/
2 lines
175 lines

Diff 35318

View Options

web/modals/threads/settings/thread-settings-delete-tab.css

Loading...
View Options

web/modals/threads/settings/thread-settings-modal.react.js

Loading...