Skip to content
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

Merged
merged 25 commits into from
Dec 30, 2020

Conversation

salarenko
Copy link
Contributor

@salarenko salarenko commented Dec 16, 2020

Please provide a link to the associated issue.

Closes: #1410

Please provide a brief summary of this pull request.

This PR:

  • Adds collapsible tabs functionality
  • Adds collapsible tabs overflow functionality
  • Adds maximum number of visible tabs functionality
  • Adds adds stacked tabs content functionality
  • Adds getElementCapacity and getElementWidth utils for counting elements capacity and width (with margins)
  • Adds a [scrollSpyDisabled] attribute to disable ScrollSpy
  • Abandons tab index based API

BREAKING CHANGES:

  • selectedIndex input has been removed, use TabPanelComponent.open()
  • selectedIndexChange output has been removed, use selectedTabChange
  • TabListComponent.selectTab has been removed, use TabPanelComponent.open()
  • TabPanelComponent.triggerExpandedPanel() has been removed, use TabPanelComponent.open()

Please check whether the PR fulfills the following requirements

Documentation checklist:

@salarenko salarenko added core Core library specific issues Fiori upgrade labels Dec 16, 2020
@salarenko salarenko added this to the Sprint 52 - Hilo milestone Dec 16, 2020
@salarenko salarenko requested a review from a team December 16, 2020 10:43
@salarenko salarenko self-assigned this Dec 16, 2020
@netlify
Copy link

netlify bot commented Dec 16, 2020

✔️ 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

@salarenko salarenko force-pushed the fix/1410-tabs-overflow-menu branch from 0b60fa0 to 665a055 Compare December 16, 2020 18:26
@salarenko salarenko changed the title [WIP] fix: (Core) tabs overflow menu fix: (Core) tabs overflow menu Dec 16, 2020
@salarenko salarenko changed the title fix: (Core) tabs overflow menu fix: (Core) Overflowing, stacked and collapsible tabs Dec 16, 2020
})
export class TabItemDirective implements CssClassBuilder, OnChanges, OnInit {
/** Apply user custom styles */
/** @hidden Apply user custom styles */
Copy link
Contributor

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 _

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change reverted

@InnaAtanasova
Copy link
Contributor

Screen Shot 2020-12-16 at 5 20 04 PM

On resizing I noticed that the More button "jumps" and goes on the second line.

@InnaAtanasova
Copy link
Contributor

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.

Copy link
Contributor

@JKMarkowski JKMarkowski left a 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,

libs/core/src/lib/tabs/tab-list.component.html Outdated Show resolved Hide resolved
libs/core/src/lib/tabs/tab-list.component.ts Outdated Show resolved Hide resolved
libs/core/src/lib/tabs/tab-utils/tab-info.class.ts Outdated Show resolved Hide resolved
@salarenko
Copy link
Contributor Author

@InnaAtanasova Thanks for the review.

  • "More v" tab not fitting into the tab panel has been fixed
  • Expand tab text can be customized, but info in the docs was missing. I've improved the docs.
  • Added required doc annotation changes

Except that I've also moved expand trigger to separate component.

@InnaAtanasova
Copy link
Contributor

Looks good to me. Thanks for addressing the comments. Awesome job!

@InnaAtanasova
Copy link
Contributor

P.S. I can still reproduce the issue with the More button going on second row when resizing the screen.

@salarenko salarenko force-pushed the fix/1410-tabs-overflow-menu branch from efbeb17 to db9b88e Compare December 18, 2020 09:48
@JKMarkowski JKMarkowski self-requested a review December 18, 2020 14:10
@salarenko
Copy link
Contributor Author

@InnaAtanasova @JKMarkowski

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.

@JKMarkowski JKMarkowski changed the base branch from master to main December 18, 2020 17:25
@mikerodonnell89
Copy link
Member

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

Screen Shot 2020-12-18 at 2 13 02 PM

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)

tabstackscroll

@salarenko
Copy link
Contributor Author

@mikerodonnell89 Thanks for the review.

  • I've improved RTL
  • Added keyboard support for expand trigger

Copy link
Contributor

@JKMarkowski JKMarkowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stefanoScalzo
Copy link
Contributor

Could u add to the programmatic selection example?

@mikerodonnell89
Copy link
Member

Pushed a fix for the bug I previously mentioned with proper tab not being highlighted when first scrolling stacked content

@mikerodonnell89 mikerodonnell89 self-requested a review December 28, 2020 18:42
@JKMarkowski JKMarkowski force-pushed the fix/1410-tabs-overflow-menu branch from dbc9c0a to e0bbef3 Compare December 30, 2020 10:52
@JKMarkowski JKMarkowski merged commit 38d3eba into main Dec 30, 2020
@JKMarkowski JKMarkowski deleted the fix/1410-tabs-overflow-menu branch December 30, 2020 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core library specific issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tabs component enhancements (overflow menu and stacking option)
5 participants