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

Fix (form range): change colors for hover and active states #1605

Merged
merged 28 commits into from
Apr 4, 2023

Conversation

isabellechanclou
Copy link
Member

@isabellechanclou isabellechanclou commented Nov 9, 2022

Related issues

Fixes issue #1320

Description

Fix colors of the grab circle on hover and active state for Chrome and Firefox.

⚠️ To know : It is impossible to change the horizontal bar color in Chrome to make it orange on a certain part. Therefore, it stays grey on all its length.

Motivation & Context

These colors changes are needed to respect the orange design system specifications.

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

@isabellechanclou isabellechanclou added v5 docs Improvements or additions to documentation fix css labels Nov 9, 2022
@isabellechanclou isabellechanclou linked an issue Nov 9, 2022 that may be closed by this pull request
3 tasks
@netlify
Copy link

netlify bot commented Nov 9, 2022

Deploy Preview for boosted ready!

Name Link
🔨 Latest commit e4e9393
🔍 Latest deploy log https://app.netlify.com/sites/boosted/deploys/642bdf4b405d610008b0e399
😎 Deploy Preview https://deploy-preview-1605--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 settings.

@julien-deramond
Copy link
Member

Haven't looked at the details of this PR but the new Sass var should be documented in the migration guide.

Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

Some things to tackle before approval!

scss/_variables.scss Outdated Show resolved Hide resolved
scss/_variables.scss Outdated Show resolved Hide resolved
scss/forms/_form-range.scss Outdated Show resolved Hide resolved
scss/forms/_form-range.scss Outdated Show resolved Hide resolved
scss/forms/_form-range.scss Outdated Show resolved Hide resolved
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
@isabellechanclou isabellechanclou force-pushed the main-ic-fix-form-range-interactive-colors branch from d34a2d1 to 5f1e17f Compare November 10, 2022 15:03
Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

Small fixes

site/content/docs/5.2/migration.md Outdated Show resolved Hide resolved
site/content/docs/5.2/migration.md Outdated Show resolved Hide resolved
site/content/docs/5.2/migration.md Outdated Show resolved Hide resolved
Co-authored-by: Louis-Maxime Piton <louismaxime.piton@orange.com>
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.

Maybe we can inform the designers that in https://system.design.orange.com/0c1af118d/p/88ab5b-forms/b/599459/i/48901789, the cursor is not the same hand cursor.

scss/_variables.scss Outdated Show resolved Hide resolved
site/content/docs/5.2/migration.md Outdated Show resolved Hide resolved
scss/forms/_form-range.scss Outdated Show resolved Hide resolved
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

js/dist/util/template-factory.js.map should be removed from the PR

scss/forms/_form-range.scss Outdated Show resolved Hide resolved
site/content/docs/5.2/migration.md Outdated Show resolved Hide resolved
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
@Franco-Riccitelli
Copy link

I’ve checked the state colours for hover, press and drag for the grab handle and it all looks good (as specified on a white background). The cursor also behaves as expected with the hand cursor changing on press and drag.

I will change the look of the cursor hand on the Sketch file for the press and drag state.

@julien-deramond
Copy link
Member

As discussed with @louismaximepiton, I've removed the deprecation of $form-range-thumb-active-border instead of adding a new $form-range-thumb-active-border-color. It will avoid a migration for our users.
Done via 6794a07

Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@julien-deramond
Copy link
Member

Can rollback it but I'm suggesting to add a "Dark variant" section in the docs with 519ce5f. Thoughts?

Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

Re thinking about this, we should remove the dark variant example for now and implement later on with a specific class

@julien-deramond
Copy link
Member

Removing dark variant since apparently this PR is not treating this use case at all. Thought that it was OK for dark variant already. And replacing the custom prop with $black. And 🚀

@sonarcloud
Copy link

sonarcloud bot commented Apr 4, 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

@julien-deramond julien-deramond merged commit 03612e8 into main Apr 4, 2023
@julien-deramond julien-deramond deleted the main-ic-fix-form-range-interactive-colors branch April 4, 2023 08:36
@julien-deramond julien-deramond mentioned this pull request Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css docs Improvements or additions to documentation fix passed design review v5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs > Forms > range: states hover and pressed/dragged incorrect
4 participants