-
Notifications
You must be signed in to change notification settings - Fork 1.2k
PatternFly 6 fixes #21943
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
base: main
Are you sure you want to change the base?
PatternFly 6 fixes #21943
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @garrett - I've reviewed your changes - here's some feedback:
Overall Comments:
- Thanks for addressing these small issues, but please consider combining related commits into a single commit to improve the commit history.
- It might be helpful to add a brief explanation of why each change is necessary in the commit message.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
These are unrelated, except that they're related to fixing issues that showed up due to PatternFly 6. (They're not necessarily PF6 bugs; just things that showed up in Cockpit since the port.)
This is literally what the commit messages do. (In other words, I still haven't found Sourcery useful.) |
@@ -304,7 +304,7 @@ const GroupsList = ({ groups, accounts, isExpanded, setIsExpanded, min_gid, max_ | |||
); | |||
|
|||
return ( | |||
<Card className="ct-card" isExpanded={isExpanded}> | |||
<Card className="ct-card card-groups" isExpanded={isExpanded}> | |||
<CardHeader actions={{ actions: tableToolbar, hasNoOffset: true }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets remove the hasNoOffset
too. Works better that way
/* Adjust the vertical alignment in the expandable card */ | ||
.card-groups { | ||
.ct-card-expandable-header { | ||
/* Align everything center vertically */ | ||
align-items: center; | ||
} | ||
|
||
.pf-v6-c-card__header-toggle { | ||
/* Use the default alignment, so it'd be center-aligned (set above) */ | ||
align-self: unset; | ||
} | ||
|
||
.pf-v6-c-content--h2 { | ||
/* Heading inside of the card brings in extra margin, which causes an offset, so we should remove it */ | ||
margin: 0; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should document this upstream in https://github.com/patternfly/patternfly
Something about align expandable icon for varying title heights. If you want I can do that
/* Rely on the margins from the log list */ | ||
&, | ||
.pf-v6-c-toolbar { | ||
padding-block-end: 0; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In v6.2.0
they have pf-m-no-padding
modifier so we should add a note about that
/* Rely on the margins from the log list */ | |
&, | |
.pf-v6-c-toolbar { | |
padding-block-end: 0; | |
} | |
// HACK: Remove once upgraded to PF v6.2.0 and Toolbar gets `hasNoPadding` | |
/* Rely on the margins from the log list */ | |
&, | |
.pf-v6-c-toolbar { | |
padding-block-end: 0; | |
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We merged v6.2.0 so hasNoPadding
should have been added now
/* Rely on the margins from the log list */ | |
&, | |
.pf-v6-c-toolbar { | |
padding-block-end: 0; | |
} |
border: none; | ||
font-size: var(--pf-t--global--font--size--md); | ||
font-family: var(--pf-t--global--font--family--heading); | ||
font-weight: var(--pf-t--global--font--weight--heading--default); | ||
font: var(--pf-t--global--font--weight--heading--default) var(--pf-t--global--font--size--heading--h2) var(--pf-t--global--font--family--heading); | ||
padding-block: var(--pf-t--global--spacer--lg) var(--pf-t--global--spacer--sm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For readability we should have them separate still
@@ -72,6 +72,13 @@ | |||
min-inline-size: 20ch; | |||
} | |||
|
|||
/* Ensure there's spacing for icons in the services switcher (failed units) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* Ensure there's spacing for icons in the services switcher (failed units) */ | |
/* Ensures spacing between text and icons in the services switcher (visible for failed units) */ |
--ct-color-list-critical-bg: var(--pf-t--global--color--nonstatus--red--100); | ||
--ct-color-list-critical-border: color-mix(in srgb, var(--pf-t--global--color--nonstatus--red--default), white 50%); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a lot of little issues and have been working on them on the side over the past few days.
Each fix is separated into a separate commit. None are really related to each other, but each is rather small and I thought they should go into a clean-up PR. We can split it apart if necessary, or keep them each as separate commits in this PR and land them together.