Shayan/webrel 2149/implement vertical tab #65
Conversation
…9/implement-vertical-tab
…9/implement-vertical-tab
| @@ -0,0 +1,107 @@ | |||
| .vertical-tab__wrapper { | |||
There was a problem hiding this comment.
minor: any reason for not putting this inside .vertical-tab ?
| } | ||
| } | ||
|
|
||
| .collapsible-vertical-tab { |
There was a problem hiding this comment.
that should be in separate file for separate component
There was a problem hiding this comment.
but in this case does it make any difference? because the consumer should use them all together. so that's why I put all styles in one file to avoid external request from consumer side
| <div | ||
| className={ | ||
| clsx('vertical-tab__item', { | ||
| 'vertical-tab__item--active': tab.title === selectedTab, |
There was a problem hiding this comment.
is tab.title an arbitrary string, or is it a specific enum?
based on the naming itself, feels like one is title/translation and the other one is enum
There was a problem hiding this comment.
it's string but not optional param.
I didn't understand the issue. could u pleas elaborate more?
| className={ | ||
| clsx('vertical-tab__item', { | ||
| 'vertical-tab__item--active': tab.title === selectedTab, | ||
| 'vertical-tab__item--disabled': tab.is_disabled |
There was a problem hiding this comment.
hmm seems like the condition for --active and --disabled is different,
does not feel good, it feels like it migth be possible to have both --disabled and --enabled class or none,
ideallly, they both should use the same flag/variable
There was a problem hiding this comment.
but they are totally different. with active we only handle the style. but with disabled we also disable the functionality for onClick as well/ that's why I used two different things.
|
🎉 This PR is included in version 1.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
changes: