feat(header): update header spacing and sizing #6661
Conversation
🦋 Changeset detectedLatest commit: e3f4ebd The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/components/src/components/post-mainnavigation/post-mainnavigation.scss
Show resolved
Hide resolved
# Conflicts: # packages/nextjs-integration/src/app/ssr/layout.tsx
alizedebray
force-pushed
the
header-spacing-2
branch
2 times, most recently
from
306d41a to
604452a
Compare
alizedebray
force-pushed
the
header-spacing-2
branch
from
604452a to
337fedd
Compare
|
.changeset/hungry-bugs-grin.md
Outdated
|
|
||
| Updated styles for elements slotted in the `post-header` component. | ||
| Slotted lists should now omit the `.list-inline` class and will be automatically styled as part of the `post-header`. | ||
| Keeping the `.inline-list` class will cause incorrect spacing between header elements. No newline at end of file |
There was a problem hiding this comment.
| Keeping the `.inline-list` class will cause incorrect spacing between header elements. | |
| Keeping the `.list-inline` class will cause incorrect spacing between header elements. |
| &:where(a, button, [role='button']), | ||
| &:where(ul) > li > :where(a, button, [role='button']) { |
There was a problem hiding this comment.
What do you mean, should we include [role="link"] and/or [tabindex="0"] as well, just to make sure they are styled correctly if developers insert such an element into a slot?
| > post-icon { | ||
| width: 1em; | ||
| height: 1em; | ||
| } |
There was a problem hiding this comment.
This does add properties, that are already defined on the post-icon by default. It only changes the specificity user would need to override the width and height properties.
| border: unset; | ||
| padding: 0.25rem 0.625rem; | ||
| gap: 0.375rem; | ||
| line-height: 1.5; |
There was a problem hiding this comment.
line-height: 1.5 is the default on the body element and is never changed in between. Defininig it does not change anything.
I see that you probably added it, to ensure it is defined correctly, in case the button-base mixin should ever define it differently.
However, if this is the only reason for its presence, we could use line-height: unset instead, so it falls back to the line-height defined on the body.
| padding: 0.25rem 0.625rem; | ||
| gap: 0.375rem; | ||
| line-height: 1.5; | ||
| font-size: 1rem; |
There was a problem hiding this comment.
While font-size: 1rem might be ok for most of the header links/buttons in the global header, it is currently not working or there is a potential risk for the folloging ones:
- Links/buttons in
post-megadropdown.
font-sizeshould change between breakpoints.
Figma Design - Links/buttons slotted in flyouts such as the user-menu, etc.
- Links/buttons slotted in the custom
navigation-controlsslot.
| gap: 0.375rem; | ||
| line-height: 1.5; | ||
| font-size: 1rem; | ||
| font-weight: 400; |
There was a problem hiding this comment.
font-weight: 400 is the default on the body element and the only reason we need to re-define it here is, because the is set to 700 by the button-base mixin.
I would suggest to do the same thing as with the line-height property. Set font-weight: unset and let it fall back to the default font-weight, defined on the body element, instead of defining an explicit value.
| @use '../functions/tokens'; | ||
| @use '../tokens/components'; | ||
|
|
||
| tokens.$default-map: components.$post-button; |
There was a problem hiding this comment.
Did you had to do this because of a technical limitation?
e.g. using the mixin in a file which defines the tokens.$default-map variable on its own and therefore overrides the one defined here?
If so, we should do the following things:
- Create a ticket to remove the
tokens.$default-mapvariable in all mixin files and instead implement the used map in the same way you did it here. - Add an ADR on github, which explaines the decision and its necessity.
📄 Description
Update header styles to match the Figma design.
This PR focuses on the
post-header, thepost-mainnavigationandpost-megadropdownwill be finalized in another PR.🚀 Demo
Updated version:
Current version to compare to:
🔮 Design review
📝 Checklist