-
Notifications
You must be signed in to change notification settings - Fork 166
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
Adjust p-nudge to better align text to the baseline grid #4957
Conversation
Adjusted the p-nudge for default text to align better to the baseline grid. Also other small fixes to align components that depend on p-nudge better to the baseline grid.
Demo starting at https://vanilla-framework-4957.demos.haus |
The problem of 1.5px being subtracted from the padding at lower pixel density screens could be circumvented by subtracting either 1.5px or 1px, depending on whether the device supports the display of a 1.5px border, by checking the pixel density of the device. padding-bottom: calc($input-vertical-padding - $input-border-thickness);
@media only screen and (max-resolution: 1.99dppx) {
padding-bottom: calc($input-vertical-padding - 1px);
} |
Fine by me, @bartaz please review |
scss/_base_forms.scss
Outdated
padding-bottom: $input-vertical-padding; | ||
padding-bottom: calc($input-vertical-padding - $input-border-thickness); | ||
|
||
@media only screen and (max-resolution: 1.99dppx) { |
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.
Why is this only inside a media query? Would such exceptions be needed anywhere else?
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.
That is true. We actually would need to do this everywhere where $input-border-thickness has to be accounted for in the components padding. I wonder if the media query should actually should just be moved to the original definition of $input-border-thickness?
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 don't think we can. SCSS variables need value defined on build time, we can't make this value dependent on media query.
If media query is needed we need to do it on component level.
But why is this value different of different dppx?
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.
Ah I see that is unfortunate.
On low pixel density devices the 1.5px border is computed as 1px.
If we use the $input-border-thickness variable in the calculation and the user is on a low pixel density screen then the border is rendered as 1px, but we still subtract 1.5px from the padding which will throw off the baseline grid alignment.
So the easiest solution I could come up with to solve this issue is to check if the screen is capable of displaying 0.5px steps (which should be everything >1.99dppx).
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.
@dgtlntv thanks, now I see you actually explained it in a comment earlier.
Overall the media query solution is not very hacky by itself, but the problem is that it needs to be done everywhere where we substract border from padding separately. Adding even more confusing complexity.
We don't seem to be doing this right now (while using 1.5px border). So would it be fine (I know it's not ideal), to keep it as is (1.5px regardless of density)? Does it throw the baseline grid off enough that it matters?
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.
They are not really connected, so we could definitely separate them out in several PR's I just combined them here, since they all relate to more accurately align to the baseline grid. But I can focus this PR on adjusting the nudge and creating a new one for the border.
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.
That is strange. Are you zoomed in by any chance? Browser zoom scales changes border values afaik.
Because onMyMachine™ it aligns perfectly both on low and high pixel density screens.
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.
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.
They are not really connected, so we could definitely separate them out in several PR's I just combined them here, since they all relate to more accurately align to the baseline grid. But I can focus this PR on adjusting the nudge and creating a new one for the border.
I have no problem with nudge value update, so if we can separate it, it can be merged right away.
As for the border adjustments between screen densities we would need to identify all the places where it's needed and find a way to make this work without too much overhead.
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 removed all non nudge related changes and will create a separate PR for the border related stuff. :)
The other proposed changes will be moved to a separate PR.
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.
Thanks
Done
0.4rem
to0.375rem
to align default text (and some headings) better to the baseline gridQA
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
Before
After