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

Convert local workarounds to PF overrides and file PF issues #686

Open
garrett opened this issue Jul 30, 2024 · 1 comment
Open

Convert local workarounds to PF overrides and file PF issues #686

garrett opened this issue Jul 30, 2024 · 1 comment
Assignees
Labels
enhancement New feature or request

Comments

@garrett
Copy link
Member

garrett commented Jul 30, 2024

In #670, we added some local CSS that fixed the icon-only buttons and toggle sizing.

We need to promote these to PF5 overrides in Cockpit and re-import that back, to replace these snippets.

We also need to make sure PatternFly has issues opened for both.

// // FIXME: Promote the CSS below to overrides, open PF issues // //

// PatternFly always adds a margin after images inside of widgets with pf-m-end, which is incorrect when it's the last element
.pf-v5-c-button__icon.pf-m-start:last-child {
    margin-inline-end: 0;
}

// PF menu toggles are no longer spaced consistently
.pf-v5-c-menu-toggle {
    padding-inline: var(--pf-v5-global--spacer--md) calc(var(--pf-v5-global--spacer--md) * 0.75);
}
@garrett garrett added the enhancement New feature or request label Jul 30, 2024
@garrett
Copy link
Member Author

garrett commented Jul 30, 2024

The first one of these issues is specifically a PF React issue as it always adds a pf-m-start and assumes there will be text after an icon. (HTML PF doesn't always add that class, so it can work fine. Therefore it's a React implementation issue.)

This image from the PatternFly website demonstrates the problem (you can see the margin to the right of the icon):

image

All I did was remove the text so it would be an icon-only button, which is similar to what we do. We wrap ours with tooltips too to make them more discoverable and accessible, but that (rightly) has no effect on the problem.

The workaround removes the margin when there is no text coming after it.


The second one is related to some changes that happened sometime in the past several months with respect to all padding and spacing within all dropdown/select type widgets in PatternFly. Only deprecated HTML versions are true to their design. Non-deprecated versions have issues with too much spacing, not enough spacing, and inconsistent spacing — sometimes even in the same widget. We've had some issues elsewhere in Cockpit related to that, and have patched it elsewhere, but this probably needs to be more globally patched.

I spoke with Matt from the PF team and he pointed me to patternfly/patternfly#6257 as the issue for this. (It suggests a lot of changes which are needed, and should fix these issues.) Unfortunately, for the time being, that will only be addressed in PatternFly 6. I'm trying to convince them to address problems like this in PatternFly 5 as well. Meanwhile, it's the issue we should link to in the override source.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants