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

Correct calculation of input padding to take real border width in con… #4954

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lyubomir-popov
Copy link
Contributor

…sideration

Done

Inpuit padding has a correctionm for border, but it uses a hard-coded 1px value. We set borders using a variable, which happens to be 1.5px. This PR corrects that error by referencing the variable instead of the hard-coded 1px value.

QA

  • Open demo
  • check buttons and inputs, verify the padding has 1.5px subtracted from the nudge value. This is a sub-pixel difference, but hsould help things adhere better to the baseline grid.
  • [Add QA steps]
  • Review updated documentation:
    • [List any updated documentation for review]

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:

  • 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

[if relevant, include a screenshot or screen capture]

@webteam-app
Copy link

Demo starting at https://vanilla-framework-4954.demos.haus

@bartaz
Copy link
Contributor

bartaz commented Jan 23, 2024

The build fails due to undefined variable. Seems that the $input-border-thickness that you try to use is defined later in the code.

It would have to be moved up. I guess it would make sense to move it to spacing file. Right now it's defined in settings_placeholders (which seems to be a bunch of weird legacy variable definitions).

@@ -117,7 +117,7 @@ $sp-after: (
$spv-nudge: map-get($nudges, p) !default; // top: nudge; bottom: unit - nudge; result: height = exact multiple of base unit
$spv-nudge-compensation: $sp-unit - $spv-nudge !default;
$input-margin-bottom: $sp-unit * 4 - $spv-nudge * 2;
$input-vertical-padding: calc($spv-nudge - 1px);
$input-vertical-padding: calc($spv-nudge - $input-border-thickness);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with this adjustment I'm not sure if the padding is correct. We have border only on the bottom, so it seems we should have calculated it separately for top and bottom padding?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants