-
Notifications
You must be signed in to change notification settings - Fork 163
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Align accordion's icon with heading #4424
Conversation
Demo starting at https://vanilla-framework-4424.demos.haus |
dd1d6aa
to
3e0897f
Compare
I updated the styling trying to follow what @lyubomir-popov shared yesterday. Now the icon's alignment is done from the baseline and we don't need media-queries. Side effect 1: since the icon is part of the "flow" now (previously it was Side effect 2: since icon size scales with font-size, the icon next to an |
$icon-top-position: calc(#{$table-cell-vertical-padding + (map-get($line-heights, h4) - $icon-size) * 0.5}); | ||
top: $icon-top-position; | ||
h4.p-accordion__heading > .p-accordion__tab::before { | ||
vertical-align: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vertical-align: 0; | |
Why is this here? It undoes the font metrics calculations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed it was an exception to align the icon to the heading. It is used in the font metrics example and defined here: https://github.com/canonical-web-and-design/vanilla-framework/blob/main/scss/_patterns_icons.scss#L62
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bartaz it tries to align with the top of lowercase letters - known as x-height. It might need a bit of finessing, but it shouldn't just be reset to 0 IMO. |
Created #4437 for further adjustments. We can merge this as it is. |
Done
- Add a media query to change the alignment of the icon when next to a heading, so we use a different line-height to calculate the icon's position depending on the screen size.Update the styling of the accordion.
Fixes #4418
QA
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:
Feature 馃巵
,Breaking Change 馃挘
,Bug 馃悰
,Documentation 馃摑
,Maintenance 馃敤
.package.json
should be updated relative to the most recent release, following semver convention:Screenshots
[if relevant, include a screenshot or screen capture]