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 Tabs/Menu/Paginator wrapping when width is too small #1973

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

Conversation

mkrecek234
Copy link
Contributor

@mkrecek234 mkrecek234 commented Jan 21, 2023

fix #1780

@mkrecek234 mkrecek234 changed the title Fix non-breaking horizontal menus on mobiles Fix non-breaking horizontal menus & tabs on mobiles Jan 21, 2023
@mvorisek
Copy link
Member

There is an fomantic/Fomantic-UI#2345 issue. How did you find the solution, any docs?

Also I wonder why this PR is lowering the coverage.

@mkrecek234
Copy link
Contributor Author

mkrecek234 commented Jan 21, 2023

@mvorisek
Copy link
Member

mvorisek commented Jan 23, 2023

When I was developing another component I now understand what stackable does.

Fomantic-UI source code:

& when (@variationButtonStackable) {
    /* --------------
       Stackable
    --------------- */

    /* Tablet Or Below */
    @media only screen and (max-width: @largestMobileScreen) {
        .ui.stackable.buttons {
            flex-direction: column;
            width: 100%;
...

the title of this PR is correct (ie. for/based on mobile devices with small viewport width), but I would say we should NOT merge it, as we do not want to stack based on viewport width, but instead on owning element width

the general usecase is limitted owning element width, like several tables (with paginator/menu) next to each other

please read fomantic/Fomantic-UI#2345 - the solution is to use wrapping class, but it is currently supported on FUI buttons, but not menu

if you would like to get this patched before fixed in FUI officially, you need to patch CSS to add wrapping class support

at least to fix #1780, stackable might be otherwise ok

@mkrecek234
Copy link
Contributor Author

@mvorisek From a UI perspective, menus should stack, not wrap from my understanding if you have small mobile screens vertically- you don‘t want more than one element per line in small viewport widths. On vertical oriented mobiles all menus are stacked in common web applications, otherwise it is not easy to be used. Wrapping would afaik line-break according to available size which can be on top for desktop screens. Nevertheless, for mobile it ahould stack.

@mvorisek @lubber-de What do you think?

src/Tabs.php Outdated
@@ -9,7 +9,7 @@
class Tabs extends View
{
public $defaultTemplate = 'tabs.html';
public $ui = 'tabular menu';
public $ui = 'tabular menu stackable';
Copy link
Member

Choose a reason for hiding this comment

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

I support this change in Tabs.

image

there is an UX issue (the tab menu is hard it understand vs. other components/text) which should be reported, but I do not object to merge it until fixed

Copy link
Contributor Author

@mkrecek234 mkrecek234 Jan 23, 2023

Choose a reason for hiding this comment

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

Why not for menu? Please try a grid with larger horizontal top menu: The rows stack perfectly, but the menu does not in current code. With stackable all fine, so I would merge both changes

Copy link
Member

Choose a reason for hiding this comment

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

see #1973 (comment)

my primary comment on this PR was in reaction on "fix 1780" description, which is definitely not fixed by this PR

stackable in Menu might be ok in some cases, but UX can be visually very different and can take much more space which might be not wanted

test demos/collection/grid.php demo - when viewport width is below stackable treshold, the table is already very badly reformat, so I would actually prefer to unstackable there

src/Menu.php Outdated
@@ -9,7 +9,7 @@

class Menu extends View
{
public $ui = 'menu';
public $ui = 'menu stackable';
Copy link
Member

Choose a reason for hiding this comment

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

such behaviour is not typical, here is an example of mobile GH

image

stackable might make UX between mobile and desktop too much different

Copy link
Contributor Author

@mkrecek234 mkrecek234 Jan 23, 2023

Choose a reason for hiding this comment

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

Indeed best would be to hide remaining menu items with three dot menu, or make it horizontally scrollable as on Amazon.de. Currently it destroys the whole UI as the menu is very wide, but the rows are stacked, so stacking at least would be consistent to rest of grid

@mvorisek mvorisek changed the title Fix non-breaking horizontal menus & tabs on mobiles Fix Tabs/Menu/Paginator wrapping when width is too small Jan 23, 2023
@mvorisek mvorisek removed the BLOCKED label Jan 23, 2023
@@ -1,4 +1,4 @@
<div class="{PaginatorType}ui pagination menu{/}">
<div class="{PaginatorType}{/}">
Copy link
Member

Choose a reason for hiding this comment

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

this was a bug, class is added by Paginator as well

basic: false,
compact: 'very',
basic: 'very',
unstackable: true,
Copy link
Member

Choose a reason for hiding this comment

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

when width is > stackable width (desktop), the format can be hard too read already, so this needs to be fixed properly in another PR

@mvorisek
Copy link
Member

@mkrecek234 please test this PR, my primary goal is to develop atk4/ui in a way unlimited components/nesting is possible, so the goal is not to focus on stackable (small viewports only)

"wrapping buttons" together with "menu" seems to be a good workaround for fomantic/Fomantic-UI#2345 and I am completely ok to merge this PR even if the visual style might be not perfect/officially supported by Fomantic-UI ATM when wrapped

@mvorisek
Copy link
Member

⚠️the coverage decrease seems to be related with this PR

@mkrecek234
Copy link
Contributor Author

mkrecek234 commented Jan 30, 2023

FUI 2.9.2 seems to be solving this both for paginator and also for menus if we add wrapping class. fomantic/Fomantic-UI#2345 (comment) Thank you @lubber-de

@mvorisek
Copy link
Member

Please investigate the coverage drop.

@mvorisek
Copy link
Member

mvorisek commented Jan 31, 2023

I have rerun the changes in separate/fresh PR and I can confirm the coverage drop:

image

@mvorisek mvorisek marked this pull request as draft January 31, 2023 15:38
@mvorisek
Copy link
Member

@mkrecek234 it is your job to answer the CI questions in order to get this PR merged. The coverage is decreased, please explain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Paginator & menu of tables/grids/cruds & Tabs not breaking properly on small width columns
2 participants