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

Standardize healthcard CSS #21044

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

Conversation

ashley-cui
Copy link
Contributor

Standardize column-gap to 8px, and row-gap to 4px.

Screenshot from 2024-09-27 01-22-23

Fixes: #20822

Standardize column-gap to 8px, and row-gap to 4px.

Signed-off-by: Ashley Cui <[email protected]>
@ashley-cui ashley-cui requested a review from garrett September 27, 2024 12:55
Copy link
Member

@garrett garrett left a comment

Choose a reason for hiding this comment

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

It seems that the base variable, which is used here, no longer exists:

image

PF now has column and row instead, it seems:

--pf-v5-l-flex--spacer--column--base: var(--pf-v5-global--spacer--lg);
--pf-v5-l-flex--spacer--row--base: var(--pf-v5-global--spacer--sm);

I think a proper fix is to reconsider our overrides. I started to look at this, but I had a migraine happen during lunch.

Meanwhile, as PF changed things which affected Cockpit, this means a few things:

  1. Regression in PF caused by a code change (probably several months ago at this point)
  2. Other parts of Cockpit are probably affected (I have seen other weird spacing elsewhere)
  3. There weren't tests that caught this

As a more proper fix would probably other parts of Cockpit, I suggest we put this in as a temporary fix while we investigate the problem and make tests.

Here's where I was investigating this: main...garrett:cockpit:wip-pf-flex-fix — I need to stop for today and come back to this. (You can look at it more if you'd like.)

Again, I think we could merge in a fix like you have here in this PR, but we should probably use shorthand, add a comment, and open an issue to investigate the underlying issue.

Comment on lines +28 to +31
.pf-v5-l-flex {
column-gap: var(--pf-v5-global--spacer--sm);
row-gap: var(--pf-v5-global--spacer--xs);
}
Copy link
Member

Choose a reason for hiding this comment

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

We usually use shorthand for things like this; for example:

Suggested change
.pf-v5-l-flex {
column-gap: var(--pf-v5-global--spacer--sm);
row-gap: var(--pf-v5-global--spacer--xs);
}
.pf-v5-l-flex {
gap: var(--pf-v5-global--spacer--xs) var(--pf-v5-global--spacer--sm);
}

However, while reviewing this, I noticed something strange. I'll post about it in the main comment.

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.

Health card design-related issues
2 participants