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

fix(global): remove woff, use where with globals, cleanup #5435

Merged
merged 1 commit into from Mar 20, 2023

Conversation

mcoker
Copy link
Contributor

@mcoker mcoker commented Mar 20, 2023

  • Fixes Remove WOFF fonts #5313 - removes support for woff font files
  • Fixes Suggested separation of normalize vs. base theme styles #2493 - separates reset from normalize styles that can be excluded independently
  • Adds :where() around global (normalize and reset) styles. This removes the specificity from these rules, so they will not fight for specificity with any selector styles with a specificity > 0 - from our components or any user overrides.
    • This would be really useful with the old "shield" styles that applied [class*=pf-c-], [class*=pf-c-]::before, [class*=pf-c-]::after { padding/margin: 0; background: transparent; }, but those were removed for v5 in another PR. However, I think this may be good practice in general for PF and users to be able to easily override any base/global styles without having to fight for specificity or put styles in an appropriate layer (in the future).
    • A potential problem I see here is if a user loads some third party library that styles h1, a, body, etc and loads PF afterward - our styles will currently override those styles (assuming our specificity is >= and the rules match 1:1), and they will not if we wrap our styles in :where(). Though I think that kind of conflict is something that should be handled on a case by case basis when importing multiple global styles as there are likely multiple conflicts like that regardless of using :where()
  • Removes bootstrap variables file
  • Removes card and button styles from light/dark placeholder. The button styles were never intended to stay there, and the card style is too specific to an old concept of a dashboard layout.
  • Renames the pf-*-no-reset.scss files to no-globals, since reset has a more specific meaning.

@patternfly-build
Copy link

patternfly-build commented Mar 20, 2023

@srambach
Copy link
Member

I see a difference in masthead -
image

@srambach
Copy link
Member

srambach commented Mar 20, 2023

There's a slightly different background on dark theme card (actually I think many dark theme) demos
image

@mcoker
Copy link
Contributor Author

mcoker commented Mar 20, 2023

There's a slightly different background on dark theme card (actually I think many dark theme) demos

@srambach that's from removing the background-color from <body> since those full page examples are on a blank page.

I see a difference in masthead -

That's to be expected, the old styles were from previous design iterations on buttons.

Copy link
Member

@srambach srambach left a comment

Choose a reason for hiding this comment

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

OK, looks good then!

@srambach
Copy link
Member

Actually though - that primary button doesn't pass the contrast check.

@mcoker
Copy link
Contributor Author

mcoker commented Mar 20, 2023

@srambach opened #5436 as a follow-up

Copy link
Contributor

@mattnolting mattnolting left a comment

Choose a reason for hiding this comment

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

LPTM 🔥

@mattnolting mattnolting merged commit 1000292 into patternfly:v5 Mar 20, 2023
@patternfly-build
Copy link

🎉 This PR is included in version 5.0.0-alpha.34 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove WOFF fonts Suggested separation of normalize vs. base theme styles
4 participants