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

Checks&Radios: Increase the size to follow the brand #1860

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

louismaximepiton
Copy link
Member

@louismaximepiton louismaximepiton commented Feb 27, 2023

Note: Please transform - [ ] into - (NA) in the description when things are not applicable

Related issues

NA

Description

Working on an example, I found this issue, so here is a separate PR to tackle this.

Motivation & Context

Checks and Radios are too small compared to the design.

Types of change

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

Live previews

Checklist

Contribution

Accessibility

  • My change follows accessibility good practices; I have at least run axe

Design

  • My change respects the design guidelines defined in Orange Design System
  • My change is compatible with responsive display

Development

  • My change follows the developer guide
  • (NA) I have added JavaScript unit tests to cover my changes
  • (NA) I have added SCSS unit tests to cover my changes

Documentation

  • My change introduces changes to the documentation and/or I have updated the documentation accordingly

Checklist (for Core Team only)

  • (NA) My change introduces changes to the migration guide
  • (NA) My new component is added in Storybook
  • (NA) My new component is compatible with RTL
  • Manually run BrowserStack tests
  • Manually test browser compatibility with BrowserStack (Chrome >= 60, Firefox >= 60 (+ ESR), Edge, Safari >= 12, iOS Safari, Chrome & Firefox on Android)
  • Code review
  • Design review
  • A11y review

After the merge

@netlify
Copy link

netlify bot commented Feb 27, 2023

Deploy Preview for boosted ready!

Name Link
🔨 Latest commit 138252d
🔍 Latest deploy log https://app.netlify.com/sites/boosted/deploys/65967b710328b70008b8c163
😎 Deploy Preview https://deploy-preview-1860--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.

scss/_variables.scss Outdated Show resolved Hide resolved
scss/_variables.scss Outdated Show resolved Hide resolved
Copy link
Contributor

@MewenLeHo MewenLeHo left a comment

Choose a reason for hiding this comment

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

A little suggestion and it's ok for me ;)

Copy link
Contributor

@MewenLeHo MewenLeHo left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@sonarcloud
Copy link

sonarcloud bot commented Mar 8, 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

@Franco-Riccitelli
Copy link

Checkboxes and Radio buttons are now the correct size. All looks good to me.

@Aniort
Copy link
Contributor

Aniort commented Apr 11, 2023

in terms of size, good to me
but i glanced at this page and I saw the chapter "star rating"
What a surprise when I saw that the unchecked stars are in gray to not contrasted enough although they convey information and are activable.
We must increase contrast!

@Aniort Aniort self-assigned this Apr 11, 2023
@Aniort Aniort removed their assignment Apr 11, 2023
@Aniort Aniort self-requested a review April 11, 2023 08:59
Copy link
Contributor

@Aniort Aniort left a comment

Choose a reason for hiding this comment

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

contrast on "star rating" chapter

@louismaximepiton
Copy link
Member Author

louismaximepiton commented Apr 11, 2023

It will be tackled in another PR. Thanks a lot for your review!

Edit: Will be tackled via #2005

@julien-deramond
Copy link
Member

IMO this PR should also tackle a change of margins for the .form-checks.
With the new size, they are really close together by default:
Screenshot 2023-04-26 at 09 24 26
Thoughts @louismaximepiton?

@louismaximepiton
Copy link
Member Author

louismaximepiton commented Apr 26, 2023

As far as I can see, it seems like 20px is common at Orange, should I set it to 20 or maybe only 10? 5? Might be asked to a designer maybe 🤔 Or maybe change the min-height to 1.875rem that would be closer to what Bootstrap propose?

Thinking out loud here but shouldn't we avoid small margin like .125rem?

@louismaximepiton
Copy link
Member Author

Designer:

I think the default vertical space between checkboxes should be 15 px.

f30721b takes care of it.

@julien-deramond julien-deramond self-requested a review June 12, 2023 11:40
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

There's at least an issue in https://deploy-preview-1860--boosted.netlify.app/docs/5.3/forms/validation/#browser-defaults where the space between the checkbox and the button seems too important (compared to https://boosted.netlify.app/docs/5.3/forms/validation/#browser-defaults).

Please double-check all places in the documentation where there are some .form-check and the surrounding spacings.

@louismaximepiton
Copy link
Member Author

louismaximepiton commented Jun 14, 2023

Should be resolved right now.
For instance checked:

Should I update any design description in order to note somewhere the spacings we agreed on ?

@sonarcloud
Copy link

sonarcloud bot commented Jun 14, 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

Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

Haven't checked everything yet but the migration guide note's missing to maybe explain to users that there's a new rendering (probably not impactful) but that there's a new rule that can be impactful regarding the fact that there's a forced bottom margin for .form-checks.

Based on the latter, in tables.md, there are a lot of <div class="form-check mb-0"> that should be modified to take into account this rule because the .mb-0 is now useless. Please check all the margin utilities used alongside the new modifications brought by this PR to remove useless classes if needed.


// Boosted mod: Remove margin when it's the last element of a form.
&:last-child {
margin-bottom: 0;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced by this rule, need to think about it. There's no such rule in Bootstrap and it might lead to discrepancies while using libraries based on Bootstrap (just by switching the CSS to Boosted one).

Copy link
Member Author

Choose a reason for hiding this comment

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

In Bootstrap the margin bottom is 2px so I think it won't break much. However, our margin-bottom not on last-child might break some designs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Need Lead Dev Review
Development

Successfully merging this pull request may close these issues.

5 participants