vanilla-framework icon indicating copy to clipboard operation
vanilla-framework copied to clipboard

Baseline border fix

Open dgtlntv opened this issue 1 year ago • 4 comments

Done

On low pixel density devices a 1.5px border would computed as 1px. If we use $input-border-thickness = 1.5px in the calculation and the user is on a low pixel density screen then the border is rendered as 1px, but we still eg. subtract 1.5px from the padding which will throw off the baseline grid alignment. So we have to check if the users screen is capable of displaying 0.5px steps at runtime. This is only possible with CSS variables. This PR uses a CSS variable for input-border-thickness and checks the devices pixelRatio to either give it a 1.5px border or a 1px border.

Additionally the calculation for taking the border width into account was moved from $input-vertical-padding to the components themselves because sometime the border has to be removed twice (button) or only once (input).

QA

  • Open demo
  • Look at any input element
  • Verify, that the correct border amount has been substracted from the padding - on both high (>1.99dppx) and low pixel density screens.

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:

  • [x] 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.

Screenshots

Before

Screenshot 2024-05-02 at 10 31 47

After

Screenshot 2024-05-02 at 10 31 08

dgtlntv avatar May 02 '24 08:05 dgtlntv

@bartaz I created a PR for the border calculations based on CSS variables as discussed in the other PR, just so we don't forget about this.

dgtlntv avatar May 02 '24 08:05 dgtlntv

There is another issue I've noticed (it's why the tests are failing). Vanilla is not supposed to produce any CSS if you just import it. You need to explicitly include something to produce CSS.

By adding media query to settings we are breaking this rule, as there is CSS (media query definition of the border width value) in settings.

I'll have to think how best to avoid it. I guess we would need to define a default value of $input-border-width in settings, but the media query to change it would need to live elsewhere (somewhere in base styles), so it has to be explicitly included with Vanilla (but doesn't leak into CSS right away).

bartaz avatar May 07 '24 12:05 bartaz

I reverted the variables back to the scss variable and renamed the css variable.

dgtlntv avatar May 08 '24 13:05 dgtlntv

Hey @dgtlntv, did you have any thoughts or updates here? Going through and trying to push along some older PRs, so just curious what the status of this is.

pastelcyborg avatar Aug 12 '24 21:08 pastelcyborg

@pastelcyborg As Bartek commented on in one of the threads there is this issue with my approach that there should not be CSS in the settings files. I don't really know how I'd go about fixing that so Bartek said he'd be looking into it.

dgtlntv avatar Aug 13 '24 08:08 dgtlntv

So this seems to have been closed accidentally. We discussed it and I was wondering if @bartaz can find a better place for that media query?

I would change the value within the querry to 2px, and if this doesn't solve our problem, I suggest simply changing the default line thickness from 1.5 to 2px, which should require no queries.

lyubomir-popov avatar Nov 07 '24 15:11 lyubomir-popov