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 font size of all caps text component #4665

Merged
merged 6 commits into from
Feb 24, 2023
Merged

Conversation

bartaz
Copy link
Contributor

@bartaz bartaz commented Feb 21, 2023

Done

  • Make sure small caps text always has correct font size.
  • Make sure muted heading uses new small caps style.
  • Rename p-text--x-small-capitalised to p-text--small-caps.
  • Deprecate old name (comments in the code and documentation).
  • Fix the u-align-text--x-small-to-default usage on new style of small caps

Fixes #4662 #4664

QA

Demo:

Check if PR is ready for release

If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
  • Vanilla version in package.json should be updated relative to the most recent release, following semver convention:
    • if CSS class names are not changed it can be bugfix relesase (x.x.X)
    • if CSS class names are changed/added/removed it should be minor version (x.X.0)
    • see the wiki for more details
  • Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.
  • Documentation side navigation should be updated with the relevant labels.

Screenshots

image

@webteam-app
Copy link

Demo starting at https://vanilla-framework-4665.demos.haus

@bartaz
Copy link
Contributor Author

bartaz commented Feb 21, 2023

@lyubomir-popov could you review percy and see if that's what we want?

I aligned p-muted-heading and p-text-x-small-capitalised (name change will be next), but in terms of text styles they are both the same now, same font size, spacings, margins, colour. This means p-muted-heading has bit more spacing from the top than before (judging on percy), and is not "muted" anymore (uses default text colour).

Do we want these two to align in such exact way?

Percy: https://percy.io/bb49709b/vanilla-framework/builds/25319749/changed/1411902602

Demo: https://vanilla-framework-4665.demos.haus/docs/examples/patterns/headings/muted

@lyubomir-popov
Copy link
Contributor

@bartaz that's fine we have p-text--muted which just sets the text colour to grey where needed. Maybe mention that in the release notes for anyone surprised who wants to make a heading grey again.

@lyubomir-popov
Copy link
Contributor

Percy looks good to me

@lyubomir-popov
Copy link
Contributor

I gave a design +1

@bartaz bartaz changed the title WIP: Fix font size of all caps text component Fix font size of all caps text component Feb 22, 2023
@bartaz bartaz added the Feature 🎁 New feature or request label Feb 22, 2023
Comment on lines +218 to +224
// deprecated: the use of .u-align-text--x-small-to-default on small caps text is deprecated
// previously small caps was implemented as x-small text variant requiring .u-align-text--x-small-to-default compensations
// but now it's implemented with default text size not requiring any compensations,
// so we need to reset the padding-top to the default value
&.u-align-text--x-small-to-default {
padding-top: map-get($nudges, p) + map-get($browser-rounding-compensations, p);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused here, if .u-align-text--x-small-to-default is deprecated, why do we need styles for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before we changed the font small caps style used to use p-text--x-small-capitalised class, that had x-small font size and uppercase letters to fake small caps. Because it was using x-small font size it required u-align-text--x-small-to-default if someone wanted to align it with standard text.

Now, small caps uses real small caps font, on default font size. So this util is not needed when used with p-text--small-caps, but people already use it in their projects. When they use it in their projects we need to "reset" it to correct value.

I see 2 ways of doing it. Either the way that is here, saying .p-text--small-caps.u-align-text--x-small-to-default reset padding top back to default (it has a benefit that we can do it together with small caps styles, but has downside that we have to hardcode the padding value to reset it back), or when where we define u-align-text--x-small-to-default add :not(.p-text--small-caps):not(.p-text--x-small-capitalised) so it doesn't work on these class names, but work on anything else as it should.

Let me know if it's clearer now, we can jump on a call to discuss it as well. Because indeed it is a bit dirty and confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok no yeah I get it! I think it's sensible the way you did it. Do we have any strategy for handling backward compatibility like this? I mean do we drop these kinds of rules in the next major version or does it stay here "forever"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We drop them in major version, because that's when we can break things and write a proper migration guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why I try to mark them with // deprecated comment in SCSS so it is easier to find. Technically there is also a @deprecated mixin, but TBH, I haven't seen a great value in it, and it complicates the code by nesting everything one more level.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please note the .u-align-text--x-small-to-default utility might still be useful because we are still using the small size for table headers.

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, I'm only deprecating it when used on p-text--x-small-capitalised (and its new name p-text--small-caps), it is still valid utility to use with other x-small text.

Copy link
Contributor

@ClementChaumel ClementChaumel left a comment

Choose a reason for hiding this comment

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

Perfect 👍️

@bartaz bartaz merged commit 826432a into canonical:main Feb 24, 2023
@bartaz bartaz deleted the text--all-caps branch February 24, 2023 09:28
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.

Rename p-text-x-small--capitalsied to something more understandable
4 participants