-
Notifications
You must be signed in to change notification settings - Fork 682
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace TabsWithOverflow with KListWithOverflow #12035
Replace TabsWithOverflow with KListWithOverflow #12035
Conversation
Build Artifacts
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the version of KDS on develop
doesn't yet have the KListWithOverflow
in it.
@MisRob -- is there any reason we're not using KDS >= v4.0.0 on develop
? If so then this is as good a place as any to make that update eh?
@nucleogenesis KDS v4 is the release containing rebranding and it will be introduced in this PR #11911, at first to the release branch (and from there merged to the develop I assume)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code switch looks super close but there is one thing that is left behind with the component swap.
The KTabsList is used in the old TabsWithOverflow and that is where we get our tab & click handling business from.
I think that might fix the styling issues as well, but if not, I hope the fix there is straightforward.
@@ -39,49 +39,42 @@ | |||
:layout8="{ span: 6 }" | |||
:layout12="{ span: 10 }" | |||
> | |||
<TabsWithOverflow | |||
<KListWithOverflow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I tested it I noticed we're missing the styles and click-ability of the section buttons. I believe this is because the TabsWithOverflow component wrapped everything within a KTabsList
.
I think that you'll need to still migrate some of that logic to this component - such as the "activeTabId" and "@click" handler and possibly the "tabsWrapper" ref.
I think that it may work by wrapping the whole <template #item="{ item }">
block with the KTabsList
component without worrying about the #tab
slot there -- but I'm not super sure on this.
tabindex="-1" | ||
class="overflow-tabs" | ||
icon="optionsHorizontal" | ||
:style="overflowButtonStyles(overflowTabs)" | ||
:style="overflowButtonStyles(overflowItems)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the overflowItems
given by the new KListWithOverflow
are the same shape as the data we were getting before because of the missing styles -- however, this may only be due to the missing KTabsList
component issue mentioned above.
@AllanOXDi @nucleogenesis I'm not familiar with the details of work, just noticed that currently it seems we're moving away from Relatedly, |
This issue is blocked by this KDS issue - some of my feedback above re: tab styles/functionality will be added directly into KDS, so once that issue is closed, we can update this PR accordingly. |
Summary
References
#12011
Reviewer guidance
Do the changes look good to you?
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)