Introduce Tab component for displaying interface in tabs. It is not used yet. It will be used in Thread Members Modal.
In the next diff, I'll replace "Thread List tabs" with this component.
The component receives tabs, information about active tab and tab setter from parent - it doesn't keep the state with current active tab itself.
Details
Details
Testing the component will be possible when it will be used in Thread Members Modal in one of next diffs.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
In the future, we can consider replacing existing "Thread List tabs" with this component.
I think the best way to start something like this is to factor out the existing logic ("Thread List tabs"), instead of duplicating it. Of course sometimes this can be hard if there are significant differences.
Can you explain your reasoning for duplicating the code instead of factoring it out?
| web/components/tabs.css | ||
|---|---|---|
| 12 ↗ | (On Diff #10138) | I know we've used this short hand a bit: https://stackoverflow.com/questions/37386244/what-does-flex-1-mean I always find myself forgetting what flex: 1 is. Does anyone else have this issue, or is it just me: @atul @palys-swm |
| web/components/tabs.react.js | ||
| 10 ↗ | (On Diff #10138) | declare type React$Node = | null | boolean | number | string | React$Element<any> | React$Portal | Iterable<?React$Node>; React node can be a string. We don't need add it here too. |
| 28 ↗ | (On Diff #10138) | could css.backgroundTabHeader be added to css.tabHeader? then we only need to toggle one class for active and inactive. |
| web/components/tabs.css | ||
|---|---|---|
| 12 ↗ | (On Diff #10138) | I like to use this shorthand as it is really useful in most of the cases: when you want to split available space according to some proportion. The only confusion might be about flex-basis, but in this use-case having flex-basis: 0 is reasonable. So the exact meaning of flex: 1 isn't usually that important because its behavior is intuitive. |
Comment Actions
Introduced Tabs.Container and Tabs.Item following Tomek solution in D3341. Updated CSS following review comments
Comment Actions
I created the following diff (D3373), that replaces "Thread List Tabs" with this component.
| web/components/tabs.react.js | ||
|---|---|---|
| 34 ↗ | (On Diff #10300) | nit: could currentTab?.props.children ?? null be an option? |
Comment Actions
I created the following diff (D3373), that replaces "Thread List Tabs" with this component.
Okay, in this case let's leave it as-is. But in the future, instead of having two diffs 1. copy-paste into new place, 2. replace in old place... it is alway better to structure as 1. factor out of old place, 2. use in new place.
| web/components/tabs.css | ||
|---|---|---|
| 28 ↗ | (On Diff #10302) | If only the color changes, we can use more specific property |
| web/components/tabs.react.js | ||
| 24–26 ↗ | (On Diff #10302) | I don't think this is a good idea to use lambda here. Instead we should use a callback, which might be more complicated in a loop. So to fix the issue, we can e.g. create a new component Header which takes id and setTab as props and sets onClick to be a callback depending on these. |