[ColorPicker] Add hex/alpha text fields #2186
Conversation
ruairiphackett
force-pushed
the
colorpicker-hex-editor
branch
from
8e89e9d to
9bd947f
Compare
|
There was a problem hiding this comment.
Thanks for all the work you've put in, it's looking great, we Polaris really appreciates it 😄
I've noticed your adding props onto state, it's an anti-pattern in react. Converting the components to a functional component should help avoid this 😄
Adding some tests would be awesome 😄 🙏
package.json
Outdated
| @@ -192,6 +192,7 @@ | |||
| "@types/react-transition-group": "^2.0.7", | |||
| "hoist-non-react-statics": "^3.3.0", | |||
| "lodash": "^4.17.4", | |||
| "tinycolor2": "^1.4.1", | |||
There was a problem hiding this comment.
Why do we need to add this package?
There was a problem hiding this comment.
Good point, I'll remove it and add the svg colour map 👍
src/utilities/color-validation.ts
Outdated
| @@ -1,5 +1,9 @@ | |||
| import {names} from 'tinycolor2'; | |||
There was a problem hiding this comment.
If this is the only reason we're using tinycolor2 we should probably just include the map ourselves rather than adding a package for it.
src/utilities/color-validation.ts
Outdated
| import {RGBColor, RGBAColor} from './color-types'; | ||
|
|
||
| export const nameHexMap: Record<string, string> = names; |
There was a problem hiding this comment.
I would wonder if we really need keyword support though, what was the reason for adding it?
There was a problem hiding this comment.
This came up as part of a conversation with @dpersing about what aspects of the online-store-web implementation to carry over.
There was a problem hiding this comment.
Is the idea that online-store-web will consume our color picker?
There was a problem hiding this comment.
The idea of the change is to make the ColorPicker accessible by adding keyboard support. There's no plan for any particular team to consume the new version, but hopefully it'll be used in favour of the old one.
There was a problem hiding this comment.
I maybe under an incorrect assumption but I suspect the majority of people don't know color keywords, apart from the basic; red, green, purple. Not the rebeccapurple, forestgreen, papayawhip, etc. Is this something being requested accessibility wise? @dpersing
I'm not saying we shouldn't have it, just curious about the context before we increase our bundle 😄
src/utilities/color-validation.ts
Outdated
| import {RGBColor, RGBAColor} from './color-types'; | ||
|
|
||
| export const nameHexMap: Record<string, string> = names; | ||
| const IS_RGB_STRING_REGEX = /rgba?\(((25[0-5]|2[0-4]\d|1\d{1,2}|\d\d?)\s*,\s*?){2}(25[0-5]|2[0-4]\d|1\d{1,2}|\d\d?)\s*,?\s*([01]\.?\d*?)?\)/i; |
There was a problem hiding this comment.
Probably not to common but this regex won't catch all RGBA values
rgba( 0255,255,255, 01);
is a valid RGB but would fail
src/utilities/color-validation.ts
Outdated
| } | ||
|
|
||
| export function isRgbString(value: string) { | ||
| return IS_RGB_STRING_REGEX.test(value); |
There was a problem hiding this comment.
Why was this regex stored as a constant and the others weren't?
There was a problem hiding this comment.
I had been using it elsewhere & removed the other uses, good catch.
| }; | ||
| } | ||
|
|
||
| // Use named export once withAppProvider is refactored away |
There was a problem hiding this comment.
We're in the process of removing all class components / withAppProvider. Could we convert this to a functional component and use the useI18n hook?
| let roundedValue = Math.round(percentage); | ||
|
|
||
| if (roundedValue !== null) { | ||
| if (roundedValue > 100) { | ||
| roundedValue = 100; |
There was a problem hiding this comment.
It might be easier to do something like
const roundedValue = clamp(Math.round(percentage), 1, 100);Or something along those lines, what do you think?
| const {color} = props; | ||
|
|
||
| const style = {backgroundColor: hsbToString(color)}; | ||
| return <div style={style} className={styles.SwatchBackground} />; |
There was a problem hiding this comment.
Just curious why this is a component rather than inline?
There was a problem hiding this comment.
I think it can be inlined looking back at it 👍
I'll make the necessary changes
| }; | ||
| } | ||
|
|
||
| // Use named export once withAppProvider is refactored away |
| type CombinedProps = TextPickerProps & WithAppProviderProps; | ||
|
|
||
| interface State { | ||
| valueHasBeenUpdatedOnce: boolean; |
There was a problem hiding this comment.
We could probably have this as a class property rather than state unless we needed it to trigger a re-render?
|
@AndrewMusgrave @tleunen thanks a million for looking at this for me!
There were a few questions relating to 6/8 digit hex, so I thought I'd bring the discussion to one place. We went with 6 digit hex to match what is currently used in both the rails and react implementations of the colour picker in the online store (specifically the one used in the theme editor). Do you think it'd be better to use a 8 digit version? @mirualves @dpersing Any thoughts on switching to 8 digit hex that would include the alpha value (representing RGBA, instead of the 6 digit hex that represents RGB) or should we keep the 2 separate as 6 digit hex and alpha percentage as they are in the designs currently? |
I'm in favor of keeping them separate. I don't think we want to assume that most merchants will be comfortable enough with CSS to know how RGBA works. In this case I think we want to do the work of parsing what most merchants will understand, versus what would be most efficient. |
|
Sorry I didn't realize this was ready for review again, re-requesting myself so I'll get to it later 😄 |
I'm in favor of keeping them separate. 8 digit hex codes aren't too common among developers from what I've seen so I image merchants without a strong understanding of CSS or color systems may be thrown off. |
|
Feel free to ping me when this is ready for a review again 😄 |
|
@AndrewMusgrave It isn't quite ready for review yet, getting there 😄 I'll ping you when it's ready. I also want to add tests before the next one 👍 |
ruairiphackett
force-pushed
the
colorpicker-hex-editor
branch
from
ff79ec5 to
0ab5a78
Compare
ruairiphackett
changed the title
|
@AndrewMusgrave This is ready for another review if you get a chance ❤️ I changed the 2 subcomponents to functional components & hopefully addressed all the other review comments. I also added tests, but let me know if they don't cover enough 👍 |
|
Apologies for the delay in working on the This PR is ready for its second code review (@AndrewMusgrave gave a first preliminary one last week). It adds the text colour input field ( I implemented the same valid colour inputs as were used in the Online Store implementation, but was wondering if we could extend it to accept more colour types as input? Currently accepted colour types3 & 6 digit hex codes:
Colour names:
To illustrate its usage, here's a quick video: https://screenshot.click/11-27-3tqli-mcdca.mp4 Adding more colour types?I was wondering if we should add to the valid colour types for the TextPicker, by adding support for:
Also, we could make the
Let me know if you have any thoughts on extending it like this or any other comments 👍 |
|
@ruairiphackett I'm thinking for this version to keep the current support, and perhaps split out the extended support into a separate PR for a later date. What do you think, @mirualves? Also, it looks great. |
By current support do you mean hex + RGB? I'll only make sure the input field accommodates the 3 digits for each RGB value, like It really looks great, @ruairiphackett! |
@mirualves any
@dpersing I'll create an issue to track this and the extended support can either be incorporated here or in a later PR depending on what is decided. |
Awesome! @ruairiphackett, to be clear, I'm referring to the characters inside the input field, without overflowing it:
|
|
@mirualves Apologies, I misunderstood! That's true, when the transparent value is present the text does overflow when entering most That behaviour isn't ideal 💯 Could this be affected by the font type/size being used also? If we make the fix above might we run into the same issue in certain instances anyway? Adding spaces, e.g. |
We can fix that by increasing the width of the entire component. By making it a bit wider, it will accommodate the RGB values, no problem. |
Cool. I wonder if we should try to maintain a square shape for the main colour picker, i.e. width equal to height for this component? |
I don't have strong option for it. It seems to work well, regardless if the shape is more squared or not. One argument for keeping the high shorter is to save screen space, but I think we can test both options in context and see how it feels. See more rectangular examples here: http://casesandberg.github.io/react-color/ |
| "textPickerAccessibilityLabel": "HEX color", | ||
| "alphaFieldAccessibilityLabel": "Alpha" |
There was a problem hiding this comment.
@dpersing Any suggestions on what a11y labels should we use here?
The first is used for the hex/colour name/rgb input field and the second is used for the alpha percentage field.
There was a problem hiding this comment.
I added these as placeholders for now.
|
@AndrewMusgrave I added a change to the component width when the alpha picker is present. This is to allow a full |
ruairiphackett
force-pushed
the
colorpicker-hex-editor
branch
from
576f92d to
fa6e5cf
Compare
src/utilities/color-validation.ts
Outdated
| } | ||
|
|
||
| export function isRgbString(value: string) { | ||
| return /rgba?\(\s*(0*((25[0-5]|2[0-4]\d|1\d{1,2}|\d\d?)\s*,\s*?)){2}(0*(25[0-5]|2[0-4]\d|1\d{1,2}|\d\d?))\s*,?\s*([01]\.?\d*?)?\s*\)/i.test( |
There was a problem hiding this comment.
This'll match incorrectly. e.g rgba(0,0,0) without conditionals (like in php) I can't suggest a fix off the top of my head, you'll probably want to look at look behind
There was a problem hiding this comment.
rgba isn't actually supported as input in this version (it may be added later), so I will change this to only match rgb strings for now.
src/utilities/color-validation.ts
Outdated
| @@ -9,3 +22,24 @@ export function isLight({red, green, blue}: RGBColor | RGBAColor): boolean { | |||
| export function isDark(color: RGBColor | RGBAColor): boolean { | |||
| return !isLight(color); | |||
| } | |||
|
|
|||
| export function isColorName(value: string) { | |||
| if (nameHexMap[value]) { | |||
There was a problem hiding this comment.
Not a strict guideline, but we usually use the boolean constructor, i.e
function isColorName(value) { return Boolean(nameHexMap[value]) }
src/utilities/color-validation.ts
Outdated
| import {RGBColor, RGBAColor} from './color-types'; | ||
|
|
||
| export const nameHexMap: Record<string, string> = names; |
There was a problem hiding this comment.
I maybe under an incorrect assumption but I suspect the majority of people don't know color keywords, apart from the basic; red, green, purple. Not the rebeccapurple, forestgreen, papayawhip, etc. Is this something being requested accessibility wise? @dpersing
I'm not saying we shouldn't have it, just curious about the context before we increase our bundle 😄
src/utilities/color-transformers.ts
Outdated
| @@ -10,6 +11,8 @@ import { | |||
| } from './color-types'; | |||
| import {compose} from './compose'; | |||
|
|
|||
| const RGB_STRING_TO_HEX_REGEX = /^rgba?[\s+]?\([\s+]?(\d+)[\s+]?,[\s+]?(\d+)[\s+]?,[\s+]?(\d+)[\s+]?/i; | |||
There was a problem hiding this comment.
Same issue as the other regex
src/utilities/color-transformers.ts
Outdated
|
|
||
| export function rgbStringToHex(value: string) { | ||
| const rgb = normalizeColorString(value).match(RGB_STRING_TO_HEX_REGEX) || [ | ||
| 'rgb(0,0,0)', |
There was a problem hiding this comment.
| 'rgb(0,0,0)', | |
| undefined, |
Since we don't use the full capture
| return rgbStringToHex(normalizedValue); | ||
| case isColorName(normalizedValue): | ||
| return nameToHex(normalizedValue); | ||
| default: |
There was a problem hiding this comment.
We can omit the default statement and the function will return undefined 😄
| const colorValue = 'white'; | ||
| const expectedHex = nameToHex(normalizeColorString(colorValue)); | ||
|
|
||
| (textField.find('input') as any).instance().value = colorValue; |
There was a problem hiding this comment.
Same comment about getDOMNode and questions about blur
There was a problem hiding this comment.
Answer about blur above, will change to getDOMNode.
There was a problem hiding this comment.
getDOMNode fails without casting, so I will change (this and all other instances) to
(textField.find('input').getDOMNode() as HTMLInputElement).value = colorValue;
There was a problem hiding this comment.
For reference a way to avoid casting here could be to use a type guard. Here's an example using instanceof 😄
const input = textField.find(‘input’).getDOMNode();
const value = input instanceof HTMLElement ? ‘’ : input.value;There was a problem hiding this comment.
🤔 Using the ternary operator here gives the further error of
Tests should not contain conditional statements.eslint(jest/no-if)
It also didn't seem to allow me to set the input node's value.
There was a problem hiding this comment.
@AndrewMusgrave I pushed a fix using casting ((textField.find('input').getDOMNode() as HTMLInputElement).value) just to get this to a place where it is ready for another review, I'm not ignoring your advice, don't worry! Just haven't been able to get an error free method to work without casting 🤔
| private handleHexChange = (hex: string) => { | ||
| const { | ||
| color: {alpha = 1}, | ||
| onChange, | ||
| } = this.props; | ||
| const newColor = hexToHsb(hex); | ||
| onChange({...newColor, alpha}); | ||
| }; |
There was a problem hiding this comment.
I'll leave this up to Miru's best judgment 😄
| }, 0); | ||
| } | ||
| } | ||
|
|
||
| render() { | ||
| const {id, color, allowAlpha} = this.props; | ||
| const {hue, saturation, brightness, alpha: providedAlpha} = color; | ||
| const {pickerSize} = this.state; | ||
| const {pickerWidth, pickerHeight} = this.state; |
There was a problem hiding this comment.
Just curious, do we need to update these values on resize?
There was a problem hiding this comment.
Good point, they should be updated 👍
| @@ -0,0 +1,95 @@ | |||
| import React from 'react'; | |||
| import {mountWithAppProvider} from 'test-utilities/legacy'; | |||
| import {Spinner} from 'components'; | |||
There was a problem hiding this comment.
| import {Spinner} from 'components'; |
This should fix CI 😄
@AndrewMusgrave Good point, I'd be up for either reducing the name map (although that might make it difficult for people to figure out what colour names will work) or removing it completely depending on what people think 💯 |
|
@AndrewMusgrave Should have those comments addressed now, thanks again |
|
Very interested to see this to completion. Anything I can do to help? |
alex-page
force-pushed
the
master
branch
2 times, most recently
from
4eab9e5 to
9e09ba7
Compare
alex-page
force-pushed
the
main
branch
7 times, most recently
from
2c6e842 to
d2611d6
Compare
|
@ruairiphackett This PR is excellent and is quite out of date. Are there any smaller pieces you could break apart and ship easily? |
@alex-page I would like to revisit this when I get some time to work on it, hopefully in the new year. |
alex-page
force-pushed
the
main
branch
5 times, most recently
from
c630336 to
ea3c495
Compare
|
@ruairiphackett going to close for now but if you want to keep working on this please reopen. |
|
Thanks for the heads up @alex-page |
|
@ruairiphackett This looks very useful... any updates about this? |
|
For anyone who had interest in this PR, it has been updated and recreated in #8294 |
WHY are these changes introduced?
Fixes https://github.com/Shopify/polaris-ux/issues/276
WHAT is this pull request doing?
How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx:🎩 checklist
README.mdwith documentation changes