Paths

Table of Contentst

[web] refactor tabs component
ClosedPublic
Actions

Authored by ginsu on Jan 4 2024, 12:22 AM.

Details

Reviewers

atul
inka
ashoat

Commits

rCOMM49714414927e: [web] refactor tabs component

Summary

In order to house the thread list in the panel component, we need to refactor the tabs component. At this moment the tabs component is made of two parts: the tabs and the tab content. The tabs component utilizes the React.Children.* API to interact with those child components. The React.Children.* API is listed as Legacy React API in the react docs and additionally for the redesign I need to decouple the tabs from the tab content to put the tabs in the panel header and the tab content in the panel body.

In order to make this refactor easy to review, I broke this down into many steps. The second step is to introduce a new tabs component which will utilize an alternate solution outlined in the react docs: https://react.dev/reference/react/Children#accepting-an-array-of-objects-as-a-prop

The structure/pattern for this solution should be pretty similar to the Panel component introduced in D10500

Afterwards I will update every tab use case to use the new tabs api

Depends on D10517

Test Plan

flow + every place that need to update to the new tabs will have their own diff with their own demo video

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Jan 4 2024, 12:22 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJan 4 2024, 12:22 AM

Harbormaster completed remote builds in B25467: Diff 35181.Jan 4 2024, 12:40 AM

ginsu requested review of this revision.Jan 4 2024, 12:40 AM

update

ginsu edited the summary of this revision. (Show Details)Jan 4 2024, 12:50 AM

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: atul, inka.

ginsu added a parent revision: D10517: [web] rename tabs to tabs legacy.

Harbormaster completed remote builds in B25469: Diff 35183.Jan 4 2024, 1:04 AM

ginsu added a child revision: D10518: [web] upate emoji avatar selection modal to use new tabs component.Jan 4 2024, 1:14 AM

ashoat added inline comments.Jan 4 2024, 5:57 AM

web/theme.css
433 ↗	(On Diff #35183)	Can we have a Linear task for this? I'm worried that relying on code comments might lead us to forget some follow-ups

Seems correct, but would appreciate a bit of clarity on use of generics in Props<T: string> to make sure I'm understanding things correctly

web/components/tabs.react.js
15 ↗	(On Diff #35183)	Might not be understanding this correctly, but does this mean every `tabItem` must have the same `id`? I'd expect `id` to be a per-item identifier, but maybe it's a more general tab "type" identifier?
44 ↗	(On Diff #35183)	Think it particularly makes sense to memoize any components which are containers for collection of other components

This revision now requires changes to proceed.Jan 4 2024, 12:03 PM

ginsu added inline comments.Jan 4 2024, 1:43 PM

web/components/tabs.react.js
15 ↗	(On Diff #35183)	This ensures that the value of `id`, `activeTab`, and what's getting in passed as the parameter to `setTab` are all the same string value/there are no spelling mistakes. For example Since we pass in the `SidebarTab` type as the argument for `TabData` we ensure that the ids of the `tabsData` match the values of the`React.useState` and if they don't then flow will throw an error
web/theme.css
433 ↗	(On Diff #35183)	https://linear.app/comm/issue/ENG-5022/chat-tabs

ginsu attached a referenced file: F1028199: Screenshot 2024-01-04 at 4.39.55 PM.png. (Show Details)Jan 4 2024, 1:44 PM

ginsu attached a referenced file: F1028191: Screenshot 2024-01-04 at 4.35.28 PM.png. (Show Details)

ashoat added inline comments.Jan 4 2024, 1:48 PM

web/theme.css
433 ↗	(On Diff #35183)	This CSS variable is not listed in that task. Can you make sure that every "TODO" you're adding to the code is explicitly documented in that task (or a similar task)?

Seems correct, but would appreciate a bit of clarity on use of generics in Props<T: string> to make sure I'm understanding things correctly

Added an explanation inline, please lmk if you have any more questions about this!

address comments

Harbormaster completed remote builds in B25523: Diff 35259.Jan 5 2024, 12:21 AM

ashoat added inline comments.Jan 5 2024, 5:44 AM

web/theme.css
433 ↗	(On Diff #35183)	Before landing, can you please link the task that explicitly mentions `--tab-background-primary-default`? Same for all of your other diffs that leave `TODO`s like this

Looks good, make sure to link Linear task before landing

This revision is now accepted and ready to land.Jan 5 2024, 11:35 AM

ginsu added inline comments.Jan 5 2024, 11:58 AM

web/theme.css
358 ↗	(On Diff #35259)	Here is the task tracking this todo: https://linear.app/comm/issue/ENG-6351/confirm-panel-light-mode-colors
366 ↗	(On Diff #35259)	Here is the task tracking this todo: https://linear.app/comm/issue/ENG-6353/confirm-the-community-header-button-light-mode-colors
423 ↗	(On Diff #35259)	Here is the task tracking this todo: https://linear.app/comm/issue/ENG-6352/confirm-the-tooltip-light-mode-colors
433 ↗	(On Diff #35259)	Here is the task tracking this todo: https://linear.app/comm/issue/ENG-5022/chat-tabs#comment-a4014c34

ashoat requested changes to this revision.Jan 5 2024, 12:16 PM

ashoat added inline comments.

web/theme.css
433 ↗	(On Diff #35259)	This isn't listed there. I wish I didn't have to check you on these... it's been three comments I've had to make now, just trying to make sure you're tracking your todos on Linear...

This revision now requires changes to proceed.Jan 5 2024, 12:16 PM

ginsu added inline comments.Jan 5 2024, 12:19 PM

web/theme.css
433 ↗	(On Diff #35259)	In D10524 (the last diff in this stack) I updated this name from `background` to `indicator` since I felt `indicator` was a more accurate descriptor. Sorry for the confusion!