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: prevent #NaN color picker values #2734

Merged

Conversation

brian-smith-tcril
Copy link
Contributor

@brian-smith-tcril brian-smith-tcril commented Oct 19, 2023

Description

Fixes #2524

This updates the picker to have a separate colorToDisplay used by the textbox, and hexColorString used by the internal picker. This allows us to have invalid color values (which happens with any string that isn't 3 or 6 characters) without blocking user input.

This way users can still:

  • paste anything in the textbox (only 6 characters are pasted)
  • backspace without issue

Deploy Preview

https://deploy-preview-2734--paragon-openedx.netlify.app/components/colorpicker/

Merge Checklist

  • If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
  • Does your change adhere to the documented style conventions?
  • Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
  • Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
  • Were your changes tested in the example app?
  • Is there adequate test coverage for your changes?
  • Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

  • Verify your changes were released to NPM at the expected version.
  • If you'd like, share your contribution in #show-and-tell.
  • 🎉 🙌 Celebrate! Thanks for your contribution.

@netlify
Copy link

netlify bot commented Oct 19, 2023

Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b14e676
🔍 Latest deploy log https://app.netlify.com/sites/paragon-openedx/deploys/6553e56aacd10f0008618406
😎 Deploy Preview https://deploy-preview-2734--paragon-openedx.netlify.app/playroom
📱 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.

@monteri
Copy link
Contributor

monteri commented Oct 26, 2023

Hello, @brian-smith-tcril!

I have looked at your solution for the #2524 and we with our team and @adamstankiewicz decided to choose your PR and close our other PR for colorpicker.

@brian-smith-tcril brian-smith-tcril force-pushed the color-picker-behavior-test branch 2 times, most recently from 7841218 to 3e48a61 Compare November 14, 2023 18:17
@brian-smith-tcril brian-smith-tcril changed the title color picker behavior test fix: prevent #NaN color picker values Nov 14, 2023
@brian-smith-tcril brian-smith-tcril force-pushed the color-picker-behavior-test branch 2 times, most recently from ca8e5fd to 89e10bf Compare November 14, 2023 21:04
Copy link

codecov bot commented Nov 14, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (dc3eb3e) 92.83% compared to head (b14e676) 92.86%.

Files Patch % Lines
src/ColorPicker/index.jsx 96.96% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2734      +/-   ##
==========================================
+ Coverage   92.83%   92.86%   +0.03%     
==========================================
  Files         235      235              
  Lines        4240     4261      +21     
  Branches     1029     1030       +1     
==========================================
+ Hits         3936     3957      +21     
  Misses        300      300              
  Partials        4        4              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@brian-smith-tcril brian-smith-tcril marked this pull request as ready for review November 14, 2023 21:44
@brian-smith-tcril
Copy link
Contributor Author

@monteri this is out of draft and ready for review now!

@brian-smith-tcril brian-smith-tcril merged commit 7305955 into openedx:master Dec 19, 2023
10 checks passed
@openedx-semantic-release-bot

🎉 This PR is included in version 21.11.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@openedx-semantic-release-bot

🎉 This PR is included in version 22.0.0-alpha.24 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

slider in color picker results in #NaNNaNNaN when sliding after entering invalid hex
5 participants