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

feat(menu): add tab overflow scrolling and shadows #2776

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

SeanSun6814
Copy link

Description

Currently, when there are too many tabs on a menu such that they exceed the width of its container width, they overflow off the page. As mentioned in issue #1501, there are two ways this could be fixed:

  • The Tab itself gets scrollable
  • An icon gets added to the ends of the Tab

In this PR (@SeanSun6814, @jessxec), we address both of them. It allows the tabs to be scrollable if it overflows. When some tabs are hidden to the left or right, there are shadows on either side indicating that more tabs can be scrolled to.

In order to activate these features, the user needs to add some more HTML divs in their code.

Specifically, to have the scroll feature, the user should wrap the menu tabs div with another div:

<div class="ui top attached tabular scroll">

To have the shadows feature, the user should add the following divs right before and after the menu tabs:

<div class="ui top attached tabular menu">
  <div class="ui attached tabular shadowHider top shadowHiderStart"></div>
  <div class="ui attached tabular shadow shadowStart"></div>

  <div class="item" data-tab="one">one</div>
  <div class="item active" data-tab="two">two</div>

  <div class="ui attached tabular shadow shadowEnd"></div>
  <div class="ui attached tabular shadowHider shadowHiderEnd"></div>
</div>

After these divs are added, the CSS added into the fomantic library will automatically apply to them.

In the future, it might be worth looking into having these features without additional user code.

Testcase

Demo before the fix (from the issue #1501): https://codesandbox.io/s/fomantic-tab-issue-v6er3?file=/src/App.js

Demo after the fix: https://jsfiddle.net/jessxec/64L293uk/31

Screenshot

Before the fix:

image

After the fix:

Fomantic_scrolling_issue-output

Closes

#1501

@auto-assign auto-assign bot requested review from ColinFrick, ko2in, lubber-de and prudho May 7, 2023 21:06
@lubber-de
Copy link
Member

lubber-de commented May 8, 2023

Thanks for investigating and proposing a possible solution. 👍
The way your approach is made is somehow not completely fitting the way FUI works or is considered to work.

Here is my experience while trying your jsfiddle:

  • The extra wrapper only seems to be needed to make the extra shadow divs work and to hide the segments top/bottom border. Tricky solution. I wonder if this is somehow working without the extra divs 🤔
  • The wrapper should not have the nearly same classname as the menu itself. confusing and does currently not fit the FUI naming logic. But this is something easily fixable/discussable
  • If extra divs are still needed/accepted, we should give them a more neutral name, because covering the left/right side could possible be something else than a shadow
  • Your approach does not work on desktop, on Edge you need to use the direction keys to scroll the menu items, on Chrome or firefox it does not work at all (the direction keys controls the cursor inside the heading instead). Only on mobile the menubar is draggable
    • on desktop, it would either need the scrollbar to be displayed (yes, ugly, i know 😉 ) or the scroll handling would need some javascript (my visual approach seen below using extra items)
  • the shadow trick, only being shown when not scrolled to the edges, is clever, but, IMHO, does not visually fit the overall tab/FUI appearance, it looks misplaced to me. We could play around using something being more "visual pleasing" (for example radial gradient, rounded corners...
  • Hiding the scrollbar does not work at all in firefox

Here is a quick draft i played around removing the extra divs and placing left/right items instead. Yes, those would need javascript, but .tab() actually is a module, so we can add such logic here. Such logic could change the scrollposition upon click (or implement a dragging functionality), which works everywhere.
It also allows to use any desired (cover-/click-) item instead of a hardcoded "shadow"
https://jsfiddle.net/lubber/y98m7u2n/3/

Again, thanks for getting into this, and lets discuss how to proceed.

Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

Just 2 places, as examples, inside your PR which need changes. Beside my other comment, you should take care of such for future changes. Those are needed at several places , which i havent marked because of my other overall comment

.ui.top.attached.tabular.menu {
overflow: visible;
width: 100%;
min-width: fit-content !important;
Copy link
Member

@lubber-de lubber-de May 8, 2023

Choose a reason for hiding this comment

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

dont use !important, use repeating classnames to increase specificity instead

}

.ui.top.attached.tabular.menu > .item.active {
border-bottom: 1px solid #fff;
Copy link
Member

@lubber-de lubber-de May 8, 2023

Choose a reason for hiding this comment

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

Use a LESS variable instead of hardcoding the value

@jamessampford
Copy link
Contributor

  • on desktop, it would either need the scrollbar to be displayed (yes, ugly, i know 😉 ) or the scroll handling would need some javascript (my visual approach seen below using extra items)

I did come up with something in #1868 that did scrolling and overflow (using dropdown) menu, depending on what was needed (auto detected if mobile) and would be useful in .tab() (though in a way could be useful to have this logic for other menus, like a header menu, which was more what my proposal was around)

@lubber-de lubber-de marked this pull request as draft August 2, 2023 06:32
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.

3 participants