Skip to content

storage: Fix section spacing #22011

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Venefilyn
Copy link
Member

@Venefilyn Venefilyn commented May 16, 2025

We had a bit too much spacing between cards that caused issues with
storage mobile-view. Reverting this to how we had it before.

This also removes the border between cards for the storage page
and instead only keeps them for the table-style of cards we have between
storage types.

Future improvements would be to

  • Remove usage of Card for Storage page sections or embrace it fully
  • Remove usage of Card for mobile-view rows and instead use Table or
    Data list component

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2366666
Signed-off-by: Freya Gustavsson [email protected]

@Venefilyn Venefilyn requested review from garrett and jelly May 19, 2025 13:03
@jelly
Copy link
Member

jelly commented May 20, 2025

This would remove the border on mobile as can be seen here is that expected?

We had a bit too much spacing between cards that caused issues with
storage mobile-view. Reverting this to how we had it before.

This also removes the border between cards for the storage page
and instead only keeps them for the table-style of cards we have between
storage types.

Future improvements would be to
- Remove usage of Card for Storage page sections or embrace it fully
- Remove usage of Card for mobile-view rows and instead use Table or
  Data list component

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2366666
Signed-off-by: Freya Gustavsson <[email protected]>
@Venefilyn Venefilyn force-pushed the storage/remove-extra-spacing branch from a9f6661 to e906b47 Compare May 20, 2025 12:11
@Venefilyn
Copy link
Member Author

This would remove the border on mobile as can be seen here is that expected?

Yeah maybe not the best. I added the border back in for table cards so will look like before but without the extra spacing at least

@@ -350,12 +350,9 @@ $phone: 767px;
}

/* FIXME: Temporary rework plain cards to make it look like how PF6 is designed, adding borders between sections */
.pf-v6-c-card.pf-m-plain + .pf-v6-c-card.pf-m-plain,
.pf-v6-l-stack__item + .pf-v6-l-stack__item > .pf-v6-c-card.pf-m-plain {
.pf-v6-c-card.ct-small-table-card {
Copy link
Member

Choose a reason for hiding this comment

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

hm that only applies to storage, interesting. I assume this still changes other projects

Copy link
Contributor

Choose a reason for hiding this comment

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

This remove borders, as the rule does not apply seemingly anymore (according to pixel tests)

I think we probably want to keep the selectors above, and just remove the padding-block-start.

Copy link
Contributor

Choose a reason for hiding this comment

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

According to pixel tests it also affects accounts, selinux, storage.

Copy link
Member Author

Choose a reason for hiding this comment

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

@KKoukiou the selectors I changed because it modifies not only the table like stuff we have that have cards for rows. But also page sections themselves.

The border between sections causes extra confusion and the spacing there doesn't matter. So the whole section can be dropped from those parts of the UI.

Does that make sense? It's just to change the selectors from everything to just card tables

Copy link
Contributor

Choose a reason for hiding this comment

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

yes now I see. agreed this is the desired change.

Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

Needs pixel tests updates.

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