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

Replace deprecated font redhat--monospace with basic PF's monospace #1178

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

skobyda
Copy link
Contributor

@skobyda skobyda commented Aug 14, 2023

PF dropped pf-v5-global--FontFamily--redhat--monospace since patternfly/patternfly@4ec21e34fc3

Example (see "Disk identifier")
Screenshot from 2023-08-14 14-59-49

@skobyda
Copy link
Contributor Author

skobyda commented Aug 15, 2023

Added pixel tests

@garrett
Copy link
Member

garrett commented Aug 16, 2023

...is this screenshot the before or after? It's not clear. It has a bunch of problems (including font ones), which is why I'm asking.


The issues related to this PR:

  • "Size" is a number; it should have a proprortional character. Does it? (It's hard to tell with just the "1" there, but I think it's supposed to have a bottom bar serif.)
  • The font looks wrong. Is it using a bold version?

It should look like this (inside the box):

image
(From https://www.patternfly.org/design-foundations/typography/#our-font-family)


I see a few additional problems:

  • What's up with the radio buttons?! The radios and text are all misaligned. Radios are too high and labels are too low.
    image

  • The split is weird with labels on the side and the entries not constrained; it visually groups the wrong elements.
    image

    • There are a few ways to fix this: Stack the items vertically instead of a split, restricting the size of the "Size" and "Cache" fields so there's more of a gap between items, increase the gap size for the split, or shrink the label of "Format" and "Bus" (which would be inconsistent).

Restricting the width (we should probably do this with size anyway, just like we do elsewhere, to help indicate the relative size):

image

Shaving down the size of the labels looks like this, which is nice, but it's inconsistent and could be a problem with translations:

image

Bumping the gap to be another 1 PatternFly spacing units (equivalent to 16px) seems like the best approach here, perhaps:

image


We'll want to open up issues for the additional set of problems and focus the rest of this PR on the font issues.

@jelly
Copy link
Member

jelly commented Aug 21, 2023

Needs a rebase for pixel test changes which landed after the Makefile update.

@skobyda
Copy link
Contributor Author

skobyda commented Aug 21, 2023

  • The font looks wrong. Is it using a bold version?

It looks like it's bold because patternfly uses RedHatMono for it, which seems to make it bold

@skobyda
Copy link
Contributor Author

skobyda commented Aug 21, 2023

@garrett to avoid bold font, we can just use basic monospace:
Screenshot from 2023-08-21 14-05-00

@skobyda
Copy link
Contributor Author

skobyda commented Aug 21, 2023

Another update, so it seems for some reason, in cockpit the class pf-v5-u-font-family-monospace uses Red Hat Mono Medium, while on patternfly website, the same font is calculateed as Red Hat Mono Light, which as 2 degrees thinner/lighter than medium, as seen at https://github.com/RedHatOfficial/RedHatFont

@skobyda
Copy link
Contributor Author

skobyda commented Aug 22, 2023

I filed an issue with PF about not being able to select a lighter font: patternfly/patternfly#5855

Until then, maybe we could work around this by defining our own font-weight, e.g. 300 (which was the value PF used for lighter font before it broke). So it could be something like

.ct-monospace-weight {
    font-weight: 300;
}

Then it would look like this:
Screenshot from 2023-08-22 12-15-22

@garrett WDYT?

@garrett
Copy link
Member

garrett commented Aug 22, 2023

Why are you doing a 1-off patch to a PatternFly problem as a local and custom ct-* class? Shouldn't that be an override if it is a problem with PatternFly?

skobyda added a commit to skobyda/cockpit that referenced this pull request Oct 23, 2023
This fixes a discussed issue[1] where Patternfly's default monospace
font gets incorrectly calculated as Red Hat Mono Light, which as 2
degrees thinner/lighter than medium.
Updating our overrides weight values to the ones used by Red Hat Mono[2]
pagckage should fix the issue,

[1] cockpit-project/cockpit-machines#1178 (comment)
[2] https://fontsource.org/fonts/red-hat-mono
martinpitt pushed a commit to cockpit-project/cockpit that referenced this pull request Oct 24, 2023
This fixes a discussed issue[1] where Patternfly's default monospace
font gets incorrectly calculated as Red Hat Mono Light, which as 2
degrees thinner/lighter than medium.
Updating our overrides weight values to the ones used by Red Hat Mono[2]
pagckage should fix the issue,

[1] cockpit-project/cockpit-machines#1178 (comment)
[2] https://fontsource.org/fonts/red-hat-mono
@skobyda
Copy link
Contributor Author

skobyda commented Oct 31, 2023

Ever since cockpit-project/cockpit#19516 was landed, the weight of of monospace now looks correct:

Screenshot from 2023-10-31 02-11-55
Screenshot from 2023-10-31 02-11-41

@martinpitt
Copy link
Member

The change looks great, but tests are a big train wreck. Due to the many affected retries? I run them once more to compare.

@martinpitt
Copy link
Member

pixel noise in the new reference. Retrying to compare which one it will be.

@martinpitt
Copy link
Member

Yup, failed again in the same way, so I take it the original refs from this PR were outdated. I pushed the new ones

@martinpitt
Copy link
Member

@jelly f38 started to consistently fail TestMachinesStoragePools.testStoragePoolsDeletion on TF. Some regression in F38? Yesterday's landed PRs still seemed fine. Do you have time to investigate?

@martinpitt
Copy link
Member

The only recent plausible F38 update is libvirt-dbus

@martinpitt martinpitt merged commit 628d6f2 into cockpit-project:main Oct 31, 2023
24 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants