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

Remove extra vertical line from Grid #1956

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

Conversation

mkrecek234
Copy link
Contributor

@mkrecek234 mkrecek234 commented Dec 23, 2022

fix #1901

@mvorisek
Copy link
Member

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.

@mvorisek mvorisek changed the title Remove Grid vertical line Remove extra vertical line from Grid Dec 23, 2022
@mvorisek mvorisek requested review from abbadon1334, georgehristov and ibelar and removed request for mvorisek December 23, 2022 09:47
@mkrecek234
Copy link
Contributor Author

@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
Copy link
Member

mvorisek commented Jan 10, 2023

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
Copy link
Contributor Author

mkrecek234 commented Jan 10, 2023

@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
Copy link
Member

mvorisek commented Jan 10, 2023

CardDeck - as the non-entity UA is now rendered using unified Menu (#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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Member

mvorisek commented Jan 17, 2023

please reread #1956 (comment), 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 mvorisek marked this pull request as draft January 31, 2023 15:38
@mvorisek
Copy link
Member

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

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.

CRUD style changed since Fomantic-UI 2.9.0
2 participants