-
Notifications
You must be signed in to change notification settings - Fork 132
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
fix: (Core) Overflowing, stacked and collapsible tabs #4120
Conversation
✔️ Deploy preview for fundamental-ngx ready! 🔨 Explore the source changes: e0bbef3 🔍 Inspect the deploy logs: https://app.netlify.com/sites/fundamental-ngx/deploys/5fec5bd6f07418000781dff7 😎 Browse the preview: https://deploy-preview-4120--fundamental-ngx.netlify.app |
0b60fa0
to
665a055
Compare
}) | ||
export class TabItemDirective implements CssClassBuilder, OnChanges, OnInit { | ||
/** Apply user custom styles */ | ||
/** @hidden Apply user custom styles */ |
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.
Why do you hide it? If it's hidden it should be prefixed with _
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.
Change reverted
I wrote in the previous PR that the Overflow button ("More") is not customizable. It could be anything or just an arrow and needs to handle the i18n. |
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.
Great Job! I left some comments regarding code / keyboard support,
@InnaAtanasova Thanks for the review.
Except that I've also moved expand trigger to separate component. |
Looks good to me. Thanks for addressing the comments. Awesome job! |
P.S. I can still reproduce the issue with the More button going on second row when resizing the screen. |
efbeb17
to
db9b88e
Compare
This commit has introduced additional wrapper on tabs. Now when tabs cannot fit when resizing they will overflow without moving the "More v" tab to second row. This diverge from Fundamental Styles structure. |
Nicely done! Couple very minor issues. I think enter key/space bar when "more" is focused should open the overflow popover. Also the icon/text needs some spacing on the more button in RTL mode When first scrolling the stacked example, the selected tab does not update. But it does on subsequent scrolls. (wondering if this issue would also be present with stacking wizard if all steps are rendered stacked on first load) |
@mikerodonnell89 Thanks for the review.
|
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.
LGTM
Could u add to the programmatic selection example? |
Pushed a fix for the bug I previously mentioned with proper tab not being highlighted when first scrolling stacked content |
dbc9c0a
to
e0bbef3
Compare
Please provide a link to the associated issue.
Closes: #1410
Please provide a brief summary of this pull request.
This PR:
getElementCapacity
andgetElementWidth
utils for counting elements capacity and width (with margins)[scrollSpyDisabled]
attribute to disableScrollSpy
BREAKING CHANGES:
selectedIndex
input has been removed, useTabPanelComponent.open()
selectedIndexChange
output has been removed, useselectedTabChange
TabListComponent.selectTab
has been removed, useTabPanelComponent.open()
TabPanelComponent.triggerExpandedPanel()
has been removed, useTabPanelComponent.open()
Please check whether the PR fulfills the following requirements
https://github.com/SAP/fundamental-ngx/blob/master/CONTRIBUTING.md
https://github.com/SAP/fundamental-ngx/wiki/PR-Review-Checklist
Documentation checklist:
README.md