Paths

Table of Contentst

Differential D3845

[web] Use new `Tabs.Container` instead of existing `Tabs` component
ClosedPublic
Actions

Authored by atul on Apr 25 2022, 4:59 PM.

Tags

None

Referenced Files

	F3376726: D3845.id11957.diff
	Wed, Nov 27, 1:50 AM

	F3376718: D3845.id12092.diff
	Wed, Nov 27, 1:49 AM

	F3376277: D3845.diff
	Tue, Nov 26, 11:04 PM

	Unknown Object (File)
	Sat, Nov 23, 9:42 AM

	Unknown Object (File)
	Sat, Nov 9, 10:48 PM

	Unknown Object (File)
	Sat, Nov 9, 7:26 AM

	Unknown Object (File)
	Fri, Nov 8, 12:34 AM

	Unknown Object (File)
	Thu, Nov 7, 11:30 PM

Subscribers

Details

Reviewers

• jacek
tomek

Commits

rCOMM8ae42797155e: [web] Use new `Tabs.Container` instead of existing `Tabs` component

Summary

Use the new Tabs.Container component in ThreadSettingsModal to get things closer visually.

There's still a good amount of styling to do here... this is just the first step

Depends on D3844

Test Plan

Here's what it looks like (for now.. will re-style momentarily):

Diff Detail

Repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Apr 25 2022, 4:59 PM

Herald added subscribers: • abosh, • karol, • adrian, ashoat. · View Herald TranscriptApr 25 2022, 4:59 PM

atul edited the test plan for this revision. (Show Details)Apr 25 2022, 5:01 PM

Harbormaster completed remote builds in B8533: Diff 11892.Apr 25 2022, 5:04 PM

atul requested review of this revision.Apr 25 2022, 5:04 PM

tomek accepted this revision.Apr 26 2022, 9:28 AM

tomek added inline comments.

web/modals/threads/thread-settings-modal.react.js
164 ↗	(On Diff #11892)	Do we need to use `form`? If yes, do we need to wrap whole tab container, or use it inside each tab?

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

tomek added inline comments.Apr 26 2022, 9:28 AM

web/modals/threads/thread-settings-modal.react.js
112 ↗	(On Diff #11892)	Should we memoize this?

atul mentioned this in D3824: [web] Lift `tabs: React.Node` from `ThreadSettingsModal` to `ConnectedThreadSettingsModal`.Apr 26 2022, 10:22 AM

rebase

Harbormaster failed remote builds in B8578: Diff 11957!Apr 26 2022, 11:01 AM

Harbormaster completed remote builds in B8578: Diff 11957.Apr 26 2022, 11:11 AM

atul added inline comments.Apr 28 2022, 3:12 PM

web/modals/threads/thread-settings-modal.react.js
112 ↗	(On Diff #11892)	It was a little tricky to do in this diff, but I think we should. The issue is that the hooks all need to "live" above the `!threadInfo` early return, which means we'd need to add a bunch of invariants and whatnot that assert `threadInfo` exists. From D3823: I was planning on doing three "passes" after the refactor to make sure everything is good since a lot of things changed/got moved around. Optimizing (avoiding unnecessary renders, memoizing things, etc.) Styling (making sure that things look consistent, styles aren't broken, etc) Testing (go through all of the ThreadSettingsModal functionality thoroughly to ensure that there aren't any regressions) Will take a look at memoizing the tabs in the first pass.
164 ↗	(On Diff #11892)	Will push the `form` tag down to child `Tab` components in a subsequent diff

rebase before landing

Harbormaster completed remote builds in B8681: Diff 12091.Apr 28 2022, 3:18 PM

Closed by commit rCOMM8ae42797155e: [web] Use new `Tabs.Container` instead of existing `Tabs` component (authored by atul). · Explain WhyApr 28 2022, 3:29 PM

This revision was automatically updated to reflect the committed changes.

atul added a commit: rCOMM8ae42797155e: [web] Use new `Tabs.Container` instead of existing `Tabs` component.

atul mentioned this in D3929: [web] Remove unused styles from `thread-settings-modal.css`.May 5 2022, 6:43 AM

atul mentioned this in rCOMMa3020d2066cd: [web] Remove unused styles from `thread-settings-modal.css`.May 5 2022, 9:17 AM

Revision Contents
Changeset List

Path

Size

web/

modals/

threads/

thread-settings-modal.react.js

101 lines

Diff 12092

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

Loading...