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

Small and large buttons with icons - adjustments #1611

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

hannahiss
Copy link
Member

@hannahiss hannahiss commented Nov 10, 2022

Related issues

Closes #1122

Description

  • set horizontal padding for small buttons to 13px instead of currently 10px - and not 15px (because in sketch horizontal padding in buttons includes 2px border). This has to be done for small buttons with or without icons
  • set horizontal padding for large buttons to 18px instead of currently 20px - and not 20px (because in sketch horizontal padding in buttons includes 2px border). This has to be done for large buttons with or without icons
  • set icon size in small buttons to 15px (.9375rem) instead of 16px (1rem) => do not lower icon of 1px vertically to preserve button size (30px)
  • change existing icon with Solaris icon (same icon but not the same layout)

Motivation & Context

Differences between deisgn and reality in Boosted

Types of change

  • Bug fix (non-breaking which fixes an issues)

Live previews

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • My change introduces changes to the migration guide
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

@hannahiss hannahiss marked this pull request as draft November 10, 2022 16:09
@netlify
Copy link

netlify bot commented Nov 10, 2022

Deploy Preview for boosted ready!

Name Link
🔨 Latest commit ea4bc7f
🔍 Latest deploy log https://app.netlify.com/sites/boosted/deploys/6596d321a908680008af950a
😎 Deploy Preview https://deploy-preview-1611--boosted.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hannahiss
Copy link
Member Author

@julien-deramond
After a few tests and exchanges with LM and Isabelle, it seems that, for the small buttons:

  • putting 15px for the icons can be problematic for centering, because the button is 30px, it doesn't fit. And in addition in the doc, users have to be told to put 0.9375rem instead of 1rem...
  • if we try to lower the text by removing the 1px offset (which is line 29 of _buttons.scss), the icon also goes down. Then, if we add a padding-bottom of 1px on the icon, this icon decreases from the bottom, and we finish with the text well aligned and the icon at 15px... :-)

If this solution (remove the offset pixel + 1px padding-bottom only for small buttons) seems good to you, how to achieve it ? It would be necessary to modify the button creation mixin and I'm not sure about the good way to do this...

@hannahiss
Copy link
Member Author

hannahiss commented Jan 13, 2023

After spec review, it has been decided:

  • to change icon size to 15px instead of 16px in all small buttons
  • not to change the positioning of the text in small buttons (see Franco's comment on issue)*
  • Franco clarified that if the icon cannot be aligned in the middle (odd size in pixels), the padding must be one pixel larger at the bottom than at the top

@hannahiss hannahiss marked this pull request as ready for review January 13, 2023 08:49
scss/_buttons.scss Show resolved Hide resolved
scss/_variables.scss Outdated Show resolved Hide resolved
scss/_buttons.scss Show resolved Hide resolved
site/content/docs/5.2/migration.md Outdated Show resolved Hide resolved
…nstead of $input-btn-padding-* and correct migration guide
@louismaximepiton
Copy link
Member

FWIW, the Quantity selector icons are 10px height and width for small buttons.

@hannahiss hannahiss marked this pull request as draft February 8, 2023 11:29
@hannahiss
Copy link
Member Author

After last meeting with Franco about small buttons, he agreed visually on a version for small buttons with icons with:

  • icon size 15px (.9375rem)
  • do not lower icon of 1px vertically to preserve button size (30px) (as i used to be)
  • change existing icon with Solaris icon (same icon but not the same layout, the one from Solaris having some space at the border of svg square) => icon result is smaller

But this version does not fit for icon buttons btn-icon: icon is not centered neither vertically nor horizontally

Other questions:

  • We need to check svg quality with Franco - for rendering - and Julien - for size: the current version has been optimized with svgo => I find the result not so great in terms of quality rendering
  • We need to check if other icons in Boosted sprite are to be changed too... And if there is some impact to this/these change(s)

# Conflicts:
#	site/content/docs/5.3/migration.md
@hannahiss
Copy link
Member Author

Last discussion with Franco:
Even though we could achieve a correct design for small buttons with icons by changing the icon with the real Solaris one, it creates problems for the version with no text: the icon with 15px x 15px size cannot be centered, neither vertically (1px more at the bottom, but we already knew this) nor horizontally (1px more at right). Thus it seems not possible to change the size to 15px instead of 16px since it is not an even number.
Design card: "Dev. feedback >> Buttons with icons"

@sonarcloud
Copy link

sonarcloud bot commented Mar 29, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@B3nz01d B3nz01d added this to In progress in 🟣 PR Board (Draft) Mar 29, 2023
@hannahiss hannahiss self-assigned this Jun 9, 2023
@louismaximepiton
Copy link
Member

FYI, Designers are working on implementing even px icons. For instance 16px instead of 15px. We might want them to finish first before implementing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress / Draft
🟣 PR Board (Draft)
In progress / Draft
Development

Successfully merging this pull request may close these issues.

Docs > Components > Buttons > With icon: Fix horizontal padding (small + large sizes) + labels
3 participants