Consistently adjust the padding for all form selects #21673
Conversation
|
Zoomed screenshot crop with a guide to show how form selects are now consistent with selects and other inputs:
(The |
|
I wonder if we can drop the right-side hardcoding and only keep the left padding. And if we should rescope to only apply to Firefox (to work around the upstream workaround until it's adjusted in PatternFly directly). And then update the comments, probably. It's something to look into tomorrow, and to report to PatternFly upstream. |
garrett
force-pushed
the
pf-override-form-select-fixup
branch
2 times, most recently
from
d0f516e to
a04f7ae
Compare
|
Oh, perhaps we still need the padding on the right for Firefox... for example:
As seen in https://issues.redhat.com/browse/RHEL-12150 |
|
Re-adding the right padding, this time the cross-browser version (
Instead of:
So then I went back to check on other browsers, and here's Chrome without any overrides:
(I checked this by changing the grid to flex in So we need to re-add the spacing for all browsers to some degree. |
garrett
force-pushed
the
pf-override-form-select-fixup
branch
2 times, most recently
from
3f1eb7a to
939284b
Compare
| --pf-v5-c-form-control--PaddingRight: calc( | ||
| var(--pf-v5-global--icon--FontSize--md) + | ||
| var(--pf-v5-global--spacer--lg) | ||
| ); |
There was a problem hiding this comment.
Do we have to use lg spacer? md should be fine from what I can see
There was a problem hiding this comment.
Spacer is for both sides. We could split it into two parts and add them together (space/2 + space/2), but I thought keeping it as icon + space was broken down enough.
There was a problem hiding this comment.
Both sides? Is that due to rtl? Otherwise I'm not following sorry
|
Nice, the pixel test failure confirms the fix. The unit tests complain about some stylelint problems, I'll fix up both. I confirm this also fixes the issue in c-podman. Treating as release-blocker, this is a nice fix. |
It seems Firefox fixed their longstanding bug, which caused the workaround in PatternFly (who adopted it from us) to no longer be relevant. I adjusted our own workaround, but it didn't apply as PatternFly also (at some point in time) changed the DOM so the selector no longer matched. We'll want to communicate this with PF to get PF6 fixed. Possibly even PF5 too, as it apparently changed in Firefox recently, and therefore newer versions of Firefox will "break" the expected layout without addressing it in PF directly (or with a workaround in codebases). Meanwhile, overriding the override by setting the left spacing to be the standard left spacing makes sense too. And we can just set this for Firefox-only instead of everything (which would still work, of course, but let's scope this to the scope of the issue related to the workaround). Fixes cockpit-project#21671 https://issues.redhat.com/browse/RHEL-12150
martinpitt
force-pushed
the
pf-override-form-select-fixup
branch
from
939284b to
56adb66
Compare
|
Fixed stylelint, accepted pixel changes, and added the two bug refs to the commit message. Cheers! 👍 |
It seems Firefox fixed their longstanding bug, which caused the workaround in PatternFly (who adopted it from us) to no longer be relevant. I adjusted our own workaround, but it didn't apply as PatternFly also (at some point in time) changed the DOM so the selector no longer matched.
We'll want to communicate this with PF to get PF6 fixed. Possibly even PF5 too, as it apparently changed in Firefox recently, and therefore newer versions of Firefox will "break" the expected layout without addressing it in PF directly (or with a workaround in codebases).
Fixes #21671