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(cxl-lumo-styles): limit secondary font style in ul to .entry-content #368

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

freudFlintstone
Copy link

As requested in cxl-wpstarter PR

Tested locally:

Screenshot from 2023-11-30 11-19-55
Screenshot from 2023-11-30 11-20-29

Copy link

github-actions bot commented Nov 30, 2023

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js 66.98 KB (+0.02% 🔺)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js 11.87 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js 28.14 KB (+0.05% 🔺)
packages/cxl-ui/pkg/dist-web/vendor.js 135.58 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js, packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js, packages/cxl-ui/pkg/dist-web/cxl-ui.js, packages/cxl-ui/pkg/dist-web/manifest.js, packages/cxl-ui/pkg/dist-web/unresolved.js, packages/cxl-ui/pkg/dist-web/vendor.js 243.72 KB (+0.02% 🔺)

@lkraav
Copy link

lkraav commented Nov 30, 2023

Yeah, in theory it will improve current state.

But we just discussed things with @heshfekry and may be opting for a deep simplification by "reset all to single typeface, expand from there".

Let's see what the decision will be there, this PR might then need to morph into just removing usage (but not definition) of secondary typeface.

Copy link

@pawelkmpt pawelkmpt left a comment

Choose a reason for hiding this comment

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

@lkraav what about global p style? It was also set to secondary font family and one of the pieces affected is course about section:

Screenshot 2023-11-30 at 20 40 32

@pawelkmpt
Copy link

Task linked: CU-86aytq9gd Fix paragraph vs element fonts

@lkraav
Copy link

lkraav commented Nov 30, 2023

what about global p style

Yep, highly likely also needs to be contained with .entry-content, but we discussed the big elephant in the room in a @heshfekry huddle today

  • in WP almost everything is a content page
  • thus has .entry-content element
  • we might need some sort of a content page categorization system, if we are to continue with multiple typefaces: for example, out of post type page, only sales pages should probably have serif content.
  • ...or we try "big single-typeface sans-serif-only reset", and start over from there looking into where and how "secondary" or "serif" can be implemented for various glorious benefits

@pawelkmpt
Copy link

what about global p style

Yep, highly likely also needs to be contained with .entry-content, but we discussed the big elephant in the room in a @heshfekry huddle today

It is already. First line could be dropped @freudFlintstone.

 {
  font-family: var(--cxl-lumo-font-secondary);
  .entry-content & {
    font-family: var(--cxl-lumo-font-secondary);
  }
  • in WP almost everything is a content page
  • thus has .entry-content element

Yeah, it's tricky.

  • we might need some sort of a content page categorization system, if we are to continue with multiple typefaces: for example, out of post type page, only sales pages should probably have serif content.
  • ...or we try "big single-typeface sans-serif-only reset", and start over from there looking into where and how "secondary" or "serif" can be implemented for various glorious benefits

Will be next step. For now let's just focus on patching recent changes.

@freudFlintstone
Copy link
Author

@pawelkmpt how are you getting that? I only see the new one:
image

If that's from your IDE? If that's a merge, there's a problem with it

@pawelkmpt
Copy link

pawelkmpt commented Dec 4, 2023

If that's from your IDE? If that's a merge, there's a problem with it

I linked to the piece of code in

@lkraav what about global p style? It was also set to secondary font family and one of the pieces affected is course about section:

I played with it myself and it seems more complex than ul, ol.

@pawelkmpt pawelkmpt force-pushed the raphael/fix/dont-override-ul-font-style branch from fa1363f to 0c023ac Compare December 4, 2023 10:13
pawelkmpt
pawelkmpt approved these changes Dec 4, 2023
@pawelkmpt pawelkmpt merged commit e4c5fa5 into master Dec 4, 2023
5 of 6 checks passed
@pawelkmpt pawelkmpt deleted the raphael/fix/dont-override-ul-font-style branch December 4, 2023 10:15
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.

3 participants