This diff updates the sidebars modal to use the new tab components. Since we decoupled the tabs from the tab content with this new tab component, the sidebars modal now needs to be responsible for rendering the correct tab content.
Depends on D10518
Details
Details
Please see the demo video below to confirm that there are no regressions
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Some thoughts inline, but good to land as is.
| web/modals/threads/sidebars/sidebars-modal.react.js | ||
|---|---|---|
| 17–18 ↗ | (On Diff #35185) | Since id isn't really a unique identifier for each tab item, could we just use re-use header? |
| 47 ↗ | (On Diff #35185) | Don't love the magic string, would've preferred some sort of enum thing... but probably fine for now. |
| 59–62 ↗ | (On Diff #35185) | Let's definitely memoize any container components to try to short-circuit re-renders. |
| web/modals/threads/sidebars/sidebars-modal.react.js | ||
|---|---|---|
| 17–18 ↗ | (On Diff #35185) | 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 |
| 47 ↗ | (On Diff #35185) | FWIW flow will throw an error if this string does not match a value listed in SidebarTab type  |