search

cockpit-project / cockpit-files

A Featureful File Browser for Cockpit (Modernized and tested version of https://github.com/45Drives/cockpit-navigator)
GNU Lesser General Public License v2.1
52 stars 26 forks source link

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

Open garrett opened 2 months ago

garrett commented 2 months ago

In https://github.com/cockpit-project/cockpit-files/pull/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 commented 2 months ago

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 https://github.com/patternfly/patternfly/issues/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.