feat(design-system): add tabs component [AR-38982] #80
Conversation
|
Question: wouldn't be easier to maintain if we keep all the tabs related components inside the ds-tabs directory? 🤔 Each sub component can be placed in its own directory as it is now 💯 |
...ign-system/src/components/ds-tabs/components/ds-tabs-indicator/ds-tabs-indicator.module.scss
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/ds-tabs/ds-tabs.stories.tsx
Outdated
Show resolved
Hide resolved
...ages/design-system/src/components/ds-tabs/components/ds-tabs-indicator/ds-tabs-indicator.tsx
Outdated
Show resolved
Hide resolved
...ign-system/src/components/ds-tabs/components/ds-tabs-indicator/ds-tabs-indicator.module.scss
Outdated
Show resolved
Hide resolved
...esign-system/src/components/ds-tabs/components/ds-tabs-dropdown/ds-tabs-dropdown.module.scss
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/ds-tabs/components/ds-tabs-dropdown/ds-tabs-dropdown.tsx
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/ds-tabs/components/ds-tabs-dropdown/ds-tabs-dropdown.tsx
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/ds-tabs/components/ds-tabs-dropdown/ds-tabs-dropdown.tsx
Outdated
Show resolved
Hide resolved
...ages/design-system/src/components/ds-tabs/components/ds-tabs-indicator/ds-tabs-indicator.tsx
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/ds-tabs/ds-tabs.stories.tsx
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/ds-tabs/ds-tabs.stories.tsx
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/ds-tabs/ds-tabs.stories.tsx
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/ds-tabs/ds-tabs.stories.tsx
Outdated
Show resolved
Hide resolved
| import type { DsTabsContentProps } from '../../ds-tabs.types'; | ||
| import styles from './ds-tabs-content.module.scss'; | ||
|
|
||
| export const DsTabsContent = ({ value, className, style, children }: DsTabsContentProps) => { |
There was a problem hiding this comment.
why do we need multiple files for all these components? can't they be in the same file like others?
it's easier to follow, especially when reviewing
There was a problem hiding this comment.
It is a good practice to keep code moduled and splitted into smaller parts for easier support. If changes will be required in future it is much easier to review isolated part
| .tabList-horizontal { | ||
| flex-direction: row; | ||
| align-items: center; | ||
| border-bottom: $border-width solid var(--color-border-main); | ||
| gap: var(--spacing-xs); | ||
| } | ||
|
|
||
| .tabList-vertical { | ||
| flex-direction: column; | ||
| align-items: stretch; | ||
| border-right: $border-width solid var(--color-border-main); | ||
| } |
There was a problem hiding this comment.
why do you need these classes? you can use the data-attrs for this
There was a problem hiding this comment.
As data-attrs are handled by the arc-ui (and possible be updated in some version) it is more reliable to create the styling class and manage it.
There was a problem hiding this comment.
I think I would vote for the data-attrs 🤔 But that's something we can agree upon after this sprint 👍
| const contextValue: DsTabsContextType = { | ||
| orientation, | ||
| size, | ||
| currentValue: value, | ||
| onValueChange, | ||
| }; |
There was a problem hiding this comment.
why do you need to pass all of this?
seems like it's just the size that you'll need deeply, and I think we can maybe try to find a better way for this
e.g. css vars, or maybe using a single scss file for everything and checking for root-${size} > .content or something
feels like this context is an unnecessary overhead
There was a problem hiding this comment.
In case we want to keep components inside tabs separated it is good approach to use context:
- values easily accessible in each component without props drilling
- props management in single place
- isolated usage of context is affecting only tabs instance itself
There was a problem hiding this comment.
My small note: I see the value in going with the CSS vars (as we only need it for styling and nothing else); it removes extra boilerplate and code. On the other hand, the React Context is pretty common use case for this kind of scenarios.
Probably, keeping those in one place and just using dynamic CSS vars would be simplest, but I can live with both 🫡
packages/design-system/src/components/ds-tabs/components/ds-tabs-tab/ds-tabs-tab.tsx
Outdated
Show resolved
Hide resolved
3 participants
Design link