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

Heading: Does not use design token for bottom margin #547

Closed
eslawski opened this issue Jun 20, 2023 · 3 comments
Closed

Heading: Does not use design token for bottom margin #547

eslawski opened this issue Jun 20, 2023 · 3 comments

Comments

@eslawski
Copy link
Contributor

Expected behavior
The bottom margin of the heading component uses a pharos css token.

Actual behavior
The bottom margin is currently hardcoded to .5em

Steps to reproduce the issue

  1. Inspect the pharos heading component

Screenshots or code
If applicable, add screenshots or code to help explain the issue.

Pharos version
v12.19.3

@eslawski eslawski added the bug label Jun 20, 2023
@daneah
Copy link
Member

daneah commented Jun 20, 2023

@eslawski is the desired outcome here to create and use a new component token that defaults to the same value? I'd be inclined to call this a feature request rather than a bug.

@daneah daneah changed the title Pharos heading component does not use css token for bottom margin Heading: Does not use design token for bottom margin Jun 20, 2023
@eslawski
Copy link
Contributor Author

TBH I just noticed that the bottom margin wasn't using our spacing tokens and thought that it was an oversight, but now I realize that might be by design. I notice now that the bottom margin is currently using em rather than rem which is what our tokens provide. Makes sense that we probably want the bottom margin to differ for the various heading sizes and what's what the em provides since it scales relative to parent text size.

If that's a correct assessment, I think we can go ahead and close this one.

@daneah
Copy link
Member

daneah commented Jun 21, 2023

You have it right that using em was intentional so that the bottom margin would grow with the font size of the heading itself. I could see that being a token nonetheless, but I don't necessarily see a clear driver to do so right at the moment. It's a good thing to recognize as a possible disparity for the future.

@daneah daneah closed this as completed Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants