Drop old pf overrides 🧹 #20982
Drop old pf overrides 🧹 #20982
Conversation
There was a problem hiding this comment.
Looking into this. I am not sure the popover auto width. I did see an issue prior, and it's not clear if the PF fix would actually fix our issue.
I need to look further, but I need top sign out for today soon. I've added it to my TODO to look at this more tomorrow.
pkg/systemd/logs.scss
Outdated
| // https://github.com/patternfly/patternfly-react/issues/5993 | ||
| .pf-v5-c-popover.pf-m-width-auto { | ||
| --pf-v5-c-popover--MaxWidth: min(300px, 90%); | ||
| } | ||
|
|
There was a problem hiding this comment.
I literally saw what looks like this bug on a recent issue in Cockpit Machines. But that's with this override. I don't know if it would be better without the override or not, or if it wouldn't change a thing.
I'll have to come back to this. 🤨
There was a problem hiding this comment.
I'm referring to this issue: cockpit-project/cockpit-machines#1794
(So this override doesn't actually fix the problem in that case, but we might need to revisit this problem and perhaps even make another override instead. Machines should probably change the way it calls that popover, but it also shouldn't happen like that by default too.)
There was a problem hiding this comment.
Ironically, that issue in Machines is lacking this CSS workaround for whatever reason, and when I add it, it fixes the problem.
As-is, without the workaround:
With the workaround:
There was a problem hiding this comment.
Related to:
- Popover: can't make it fit inside a mobile screen patternfly/patternfly#4754
- [Popper] - [ investigate enable flip behavior for diagonal positioning] patternfly/patternfly-react#7762
It's supposed to be fixed, but it's still broken, so I don't know if we need to fix it here or elsewhere... but it does need to be fixed somewhere.
There was a problem hiding this comment.
Uh, so we make this a global override? Imo its not great either
There was a problem hiding this comment.
I agree that none of these options are great. But this is what PatternFly is giving us.
I'd personally implement this in a different way that wouldn't require so much JS and even so much CSS and DOM to do the same thing, but... (It's obvious how it should be, but this isn't done in that way, therefore it has lots of bugs like this.)
There was a problem hiding this comment.
The end result is that people using the software would care if they can actually read the text or not. Screenshot 1 is clearly wrong and bad, whereas screenshot 2 is actually functional. Regardless of implementation.
There was a problem hiding this comment.
But, meanwhile, it would be completely fine to merge this PR without this change. This change might be needed, and may even need to be extended (as seen above).
Unsetting makes no difference as the height does not change in practice.
This was fixed in PatternFly already and in machines. cockpit-project/cockpit-machines#1330 Related: cockpit-project#20973
This was fixed in PatternFly in patternfly/patternfly#6397 Related: cockpit-project#20973
jelly
force-pushed
the
drop-old-pf-overrides
branch
from
1e83be0 to
19c1766
Compare
This drops a tiny part of our overrides.