atk4 / ui

Robust and easy to use PHP Framework for Web Apps
https://atk4-ui.readthedocs.io
MIT License
440 stars 105 forks source link

Fix Tabs/Menu/Paginator wrapping when width is too small #1973

Open mkrecek234 opened 1 year ago

mkrecek234 commented 1 year ago

fix #1780

mvorisek commented 1 year ago

There is an https://github.com/fomantic/Fomantic-UI/issues/2345 issue. How did you find the solution, any docs?

Also I wonder why this PR is lowering the coverage.

mkrecek234 commented 1 year ago

Please check https://fomantic-ui.com/collections/menu.html#stackable

mvorisek commented 1 year ago

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 https://github.com/fomantic/Fomantic-UI/issues/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 commented 1 year ago

@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?

mvorisek commented 1 year ago

@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 https://github.com/fomantic/Fomantic-UI/issues/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 commented 1 year ago

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

mkrecek234 commented 1 year ago

FUI 2.9.2 seems to be solving this both for paginator and also for menus if we add wrapping class. https://github.com/fomantic/Fomantic-UI/issues/2345#issuecomment-1409042396 Thank you @lubber-de

mvorisek commented 1 year ago

Please investigate the coverage drop.

mvorisek commented 1 year ago

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

image

mvorisek commented 5 months ago

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