atk4 / ui

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

Remove extra vertical line from Grid #1956

Open mkrecek234 opened 1 year ago

mkrecek234 commented 1 year ago

fix #1901

mvorisek commented 1 year ago

I spent in this issue quite a lot of time already, testing different types of segment. When removed completely like now, there is no spacing when there are multiple CRUDs after each other. I would be happy if @ibelar can review this PR if consistent with our styling or not.

mkrecek234 commented 1 year ago

@mvorisek The vertical line is really a disturbance, whereas your spacing issue with two consecutively rendered grids is an edge case. If you please a header between two grids, you also do not have any spacing issue. Spacing issues are present all-over in FUI, eg, try placing Label in the neighbourhood of headers. I would opt for merging this PR, as we did not have any improvement or feedback over the past 3 weeks.

mvorisek commented 1 year ago

The exact issue is with CardDeck. I completely agree the line should not be there, but this PR must address the segment problems within the whole repo, otherwise we does not really fix anything, we only introduce inconsistency.

mkrecek234 commented 1 year ago

@mvorisek I tried to reproduce the issue you mentioned rendering a grid with immediately afterwards a CardDeck applying the PR (so removing the segment as in the PR). I cannot see any spacing issue, it renders perfectly fine, both in desktop and responsive/mobile view with the recent develop.

image

mvorisek commented 1 year ago

CardDeck - as the non-entity UA is now rendered using unified Menu (https://github.com/atk4/ui/pull/1965), I mentioned CardDeck here as there is the same problem :)

spacing - not after another Crud, but after/before an image with zero margin/padding for example

mkrecek234 commented 1 year ago

Do you have any repro code/example? I don't fully understand what the issue is removing the vertical segment line. I also checked all other FUI segment types, but no one is appropriate, so why do we need it?

mkrecek234 commented 1 year ago

@ibelar Can you check this, please? The vertical line is a big waste of space, and to me the PR solves this issue while not generating - at least in all my UI settings - any unwanted renderings.

mkrecek234 commented 1 year ago

@mvorisek Do you have any repro code, as I cannot reproduce the error. I think images should be placed in separate containers to fit them into the UI flow, this to me is not related to having the vertical line segment or not as far as I can see. I tested against headers, further Grids/Cruds or CardDecks following an the UI without vertical segment is fine.

mvorisek commented 1 year ago

please reread https://github.com/atk4/ui/pull/1956#issuecomment-1377195125, repro https://dev.agiletoolkit.org/demos/collection/card-deck.php


and the segment class is present even on more placeces


repro for removed segment

for example demos/collection/crud.php

image removing segment as proposed here (screenshot with divider removed) introduce spacing problem (as I was expecting above with image)

mvorisek commented 7 months ago

I would like the issue solved, but as shown in the discussion above, the solution as proposed ATM is causing side effects.