dcc redesign: tabs #3484
dcc redesign: tabs #3484
Conversation
KoolADE85
requested review from
T4rk1n,
emilykl,
gvwilson and
ndrezn
as code owners
KoolADE85
force-pushed
the
feature/dcc-refactor-tabs
branch
from
522b8c9 to
577158a
Compare
| const tabContainerClassNames = [ | ||
| 'tab-container', | ||
| vertical ? 'tab-container--vert' : null, | ||
| props.className, | ||
| ].filter(Boolean); | ||
|
|
||
| const tabContentClassNames = [ | ||
| 'tab-content', | ||
| vertical ? 'tab-content--vert' : null, | ||
| props.content_className, | ||
| ].filter(Boolean); | ||
|
|
||
| const tabParentClassNames = [ | ||
| 'tab-parent', | ||
| vertical ? ' tab-parent--vert' : null, | ||
| isAboveBreakpoint ? ' tab-parent--above-breakpoint' : null, | ||
| props.parent_className, | ||
| ].filter(Boolean); |
There was a problem hiding this comment.
Not sure this filter pattern is very efficient compared to regular if += in a useMemo in a direct string concat
There was a problem hiding this comment.
I started using this pattern since I noticed a few places where null & undefined became part of the className. This was a compact way to prevent that.
Also, it seems more readable to me to have one className per line instead of string concats with ternary operators (and prettier somehow makes that "uglier", lol).
I put it in a useMemo, as you suggested. What do you think of this? If you still prefer an if statement for each, I'm happy to switch to that format, but it's twice as many code lines.
There was a problem hiding this comment.
It's more about the performance, altho it probably doesn't matter much in this case, but in this you end up with 2x array traversal (filter+join) vs a couple ifs, the footprint is just not on the same level of complexity.
| export interface DashComponentApi { | ||
| ExternalWrapper: typeof ExternalWrapper; | ||
| DashContext: typeof DashContext; | ||
| useDashContext: typeof useDashContext; | ||
| getLayout: (componentPathOrId: DashLayoutPath | string) => any; | ||
| stringifyId: typeof stringifyId; | ||
| } |
There was a problem hiding this comment.
The branch is out of sync with dev, the api changed and devtool was added. Might be worth to sync the branches now to prevent further conflicts down the line.
There was a problem hiding this comment.
Good call. I synced with dev and updated the type here.
2 participants
This PR refactors Tabs to follow the same pattern as the other DCC components: Typescript, function components, with CSS in a sidecar file.
A few other improvements along the way:
dash-rendererexports Typescript interface forwindow.dash_component_api(Tabs uses this api internally)DashComponentas a prop type (in addition to a genericReactNode)extract-meta.jscomprehends this the same way asReactNodeDashComponent.props,namespace, etc.