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

✨ Enable Tabs horizontal overflow #1962

Merged
merged 11 commits into from
Feb 16, 2022

Conversation

oddvernes
Copy link
Collaborator

@oddvernes oddvernes commented Feb 8, 2022

resolves #1650

  • Added scrollable prop to add overflow-x: auto on Tabs.List (default its overflow-x: hidden
  • added scroll-snap properties to Tabs.List and tabs.Tab, so that next/prev buttons can be more easily implemented by just using element.scrollTo.
  • Fixed an internal focus issue that prevented clicking on tabs when active tab has been scrolled out of view (the event listener state juggling might be a bridge too far though, hehe).
  • Added a story showing how to add prev/next buttons on an overflowing Tabs list.
  • Added a story with scrollable on an overflowing Tabs list.
  • Added a story with scrollable on an overflowing Tabs list with custom styled scrollbar (for the sake of the pull request and discussion, I guess we might just go for one solution and delete the other.

Debatable decitions:

  • I use scroll-behavior: smooth on the tabslist (unless user have prefers-reduced-motion) so scrolling is animated. I think it makes it easier keep track of the context-change visually, but i realize there are no animations otherwise in eds.
  • I hide scrollbar (even when scrollable is set) on touch devices, as i think it gets in the way of swipe gesture and reduces the user experience.

Issues with mac (not present on windows 10) that I am currently trying to fix:
chrome/firefox:

  • "Overflow with standard scrollbar" example, the scrollbar seems buggy and does not show on hover (the customized scrollbar example works fine)

Safari:

  • Arrow-navigation does not work, but doesnt work in current develop either. The keydown event is never triggered. Instead the container is scrolled since it is never preventDefaulted
  • same scollbar issue as chrome where default scrollbar doesn't appear on hover but custom scrollbar works
  • scroll-behavior: smooth is not supported (but can be enabled in experimental features so maybe it is coming)

@oddvernes oddvernes force-pushed the OOE-1650-tabs-horizontal-overflow branch 2 times, most recently from 21950b9 to da83bb4 Compare February 9, 2022 10:56
@oddvernes oddvernes force-pushed the OOE-1650-tabs-horizontal-overflow branch from da83bb4 to 86fcbc0 Compare February 9, 2022 10:56
@oddvernes oddvernes force-pushed the OOE-1650-tabs-horizontal-overflow branch from 86fcbc0 to 22e91c5 Compare February 9, 2022 13:13
@oddvernes oddvernes marked this pull request as ready for review February 9, 2022 15:23
@mimarz
Copy link
Contributor

mimarz commented Feb 14, 2022

Looks good code wise, there is a lot to unwrap 😅

I think the animation looks fine, but I think we need to have a review with @equinor/eds-ux on the behaviour and design (with the scrollbar) and such.

Speaking of scrollbar, maybe we should just design our own? The placement if feel messes visually with the "underline" we have on each Tab.

Copy link
Contributor

@mimarz mimarz left a comment

Choose a reason for hiding this comment

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

Found on visual bug with focus.

Before:
image

After:
image

@oddvernes
Copy link
Collaborator Author

oddvernes commented Feb 14, 2022

ing of scrollbar, maybe we should just design ou

This is what default scrollbar looks like on windows, and below a narrower custom scrollbar with a little bit of padding
What is the opinion of @equinor/eds-ux on this?

bilde

@oddvernes
Copy link
Collaborator Author

oddvernes commented Feb 14, 2022

Found on visual bug with focus.

Fixed by making outline-offset negative outline-width. I expect this to be further improved by making focus ring 2px wide everywhere we use dashed :focus-visible

@lucasveil
Copy link
Contributor

Approved from design side. Great work :)
Give me a heads up when it is merged so i can update ZeroHeight documentation!

Copy link
Contributor

@mimarz mimarz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@oddvernes oddvernes merged commit 364cd31 into develop Feb 16, 2022
@oddvernes oddvernes deleted the OOE-1650-tabs-horizontal-overflow branch February 16, 2022 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Next and previous icon button for navigation in Tabs
3 participants