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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃 Apply PF4 style to page titles #3367

Merged
merged 3 commits into from
May 25, 2023

Conversation

josemigallas
Copy link
Contributor

@josemigallas josemigallas commented May 18, 2023

THREESCALE-9647: Move page headers into a PageSection component

Apply Patternfly 4 style to page titles according to Page and Text components. Highlights:

  • Pages already implemented with Patternfly React now also render their own page titles. This allow the upper PageSection to get PF styles by default. This caused a difference in background-color which has been also fixed.
  • Legacy headers are inconsistent: some are h1 and others h2 and there are also weird hybrids. The

    below some are also problematic since they inherit from the common.scss. By creating a super specific rule we can override the legacy styles and use values copied from PF variables.

  • The default font sizes main-title-font-size and secondary-title-font-size have been updated with PF4 values. This affect all titles in the app, which is very convenient to maintain proportion between page header titles and body subtitles (our size for subtitles coincides with PF4 main title so they would look the same).

Before (custom h1):
Screenshot 2023-05-18 at 17 43 19

After (PF4 h1):
Screenshot 2023-05-18 at 17 48 34

@josemigallas josemigallas requested a review from a team May 18, 2023 15:40
@josemigallas josemigallas self-assigned this May 18, 2023
@mayorova
Copy link
Contributor

What is the purpose of this?
I mean, it's nice, but the drawback is that now React and non-React pages have different headers styles... which doesn't look good, IMO.

Maybe then we need to change the style of the non-React pages as well, so it looks consistent?

@mayorova
Copy link
Contributor

The Active Docs one doesn't look great, see:
Screenshot from 2023-05-19 11-27-45
There is some weird margin around the whole page, which is also of a slightly different gray color
Screenshot from 2023-05-19 11-29-22

Compared to the current:
Screenshot from 2023-05-19 11-28-02

@josemigallas josemigallas changed the title 馃 Apply PF4 style to page titles 馃毀馃 Apply PF4 style to page titles May 19, 2023
@josemigallas josemigallas marked this pull request as draft May 19, 2023 14:41
@josemigallas josemigallas changed the title 馃毀馃 Apply PF4 style to page titles 馃 Apply PF4 style to page titles May 23, 2023
@josemigallas josemigallas marked this pull request as ready for review May 23, 2023 14:38
lvillen
lvillen previously approved these changes May 23, 2023
@josemigallas
Copy link
Contributor Author

What is the purpose of this? I mean, it's nice, but the drawback is that now React and non-React pages have different headers styles... which doesn't look good, IMO.

Maybe then we need to change the style of the non-React pages as well, so it looks consistent?

@mayorova After some consideration and experimentation it's just not possible to do this in a clean way, that is without opening a huge PR that will take a long time to finish.

The fact is that our UI is already inconsistent, it's been for a while. And the only sane way to update it is little by little. This PR should not introduce any new bugs (I fixed those that you pointed out in your screenshots) but take those pages that are in "no man's land" and transition them to 100% Patternfly.

I've tried to change all headings but it's not easy since Patternfly also relies on a certain HTML structure and I don't think it's a good idea to keep introducing intrusive and hardcoded css. It's already in a very bad shape.

@mayorova
Copy link
Contributor

Just a small suggestion to make the different titles look almost the same: https://github.com/3scale/porta/pull/3378/files

I agree that we need to align everything and ideally get rid of most customizations.
But I think this is not a huge change, we can apply it in the meanwhile.

Copy link
Contributor

@lvillen lvillen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 馃挴

$hero-title-font-size: 3 * $fontSize;
$main-title-font-size: 2 * $fontSize;
$secondary-title-font-size: (3/2) * $fontSize;
$main-title-font-size: 1.5rem; // --pf-c-content--h1--FontSize
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would rem always be 16px?... I'm just wondering - I have no idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rem is a relative to the root's element font-size. It's basically what was being achieved by using n * $fontSize... So as long as the initial font-size does not change, 1rem will be worth 8px.
NOTE: I haven't found where this 8px are defined, I know PF supports defining a custom --pf-global--root--FontSize that will apply to all rems.. but we don't use that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... Last time I checked, rem is actually 16px, and it was the browser default (at least in Chrome)
So, main title would be 24px, which is what PF4 uses for h1 (so, it's OK).

Just wondering whether it makes sense using rem instead of px. But I guess it's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's 16, sorry. I think we should use whatever PF uses.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's 16, sorry. I think we should use whatever PF uses.

Yeah, that was my point. Why use 1.5rem instead of 24px (as PF4 defines it as 24px) ?

Copy link
Contributor

@mayorova mayorova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now 馃憤

@josemigallas josemigallas merged commit 3470828 into master May 25, 2023
15 of 21 checks passed
@josemigallas josemigallas deleted the THREESCALE-9647_page_headers branch May 25, 2023 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants