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

Chrome hex input bug #432

Closed
defue opened this issue Oct 17, 2017 · 12 comments · Fixed by #461
Closed

Chrome hex input bug #432

defue opened this issue Oct 17, 2017 · 12 comments · Fixed by #461

Comments

@defue
Copy link

defue commented Oct 17, 2017

If you input #fafa color into hex field of chrome color picker it will switch to rgba mode and put the #fafa into R field.

The desired behavior is to stay in the hex mode to continue inputting the rest characters of the color (#fafafa for example).

The root cause of the issue is that last versions of tinycolor (1.4.1 for example) treats #fafa as a valid color that equals to rgba(255, 170, 255, 0.6666) and therefore Chrome colorpicker automatically switches to rgba mode. The older versions (like 1.1.2 that is used on the demo page) treat #fafa color as non-valid and therefore Chrome color picker works fine.

According to the latest specs (bgrins/TinyColor#82) #fafa is a valid 4 hex short format.

To workaround the issue I commented out "this.setState({ view: 'rgb' });" lines in componentWillReceiveProps methods of ChromeFields class in ChromeFields.js.

@nathggns
Copy link

Also facing this issue.

@sophiemoire
Copy link

I noticed the same thing ! I also noticed that once it is switched to rgba mode, you cannot go back to Hex mode, you can only switch between rgba and hsla.

I read the way you workaround this issue, but it just disables the rgb view, right ? It doesn't really fix the problem...

@elvisvoer
Copy link
Collaborator

elvisvoer commented Oct 26, 2017

@sophiemoire no, that just disables the automatic switch to rgb mode when entering hex4 (#rgba) or hex8 (#rrggbbaa).

Considering that TinyColor now supports hex8 format I think the solution is to

  1. either remove the entire
    componentWillReceiveProps(nextProps) {
    and fully support hex8 (e.g. make the alpha slider change the aa value).
  2. or, consider hex4 and hex8 invalid by changing
    isValidHex(hex) {

I guess the best way to go with a quick fix is the later so you can have something like:

isValidHex(hex) {
    return hex.length !== 5 && hex.length < 8 && tinycolor(hex).isValid()
},

@elvisvoer
Copy link
Collaborator

elvisvoer commented Oct 26, 2017

There's also an issue related to EditableInput having the logic to update its value on blur. I think for the inputs on the pickers there should be a disableBlurValue prop which makes the input to update its value on componentWillReceiveProps.

I am happy to make a PR tomorrow with a fix.

Edit: looks like the EditableInput focus is done on purpuse according to this: #398

@nathggns
Copy link

nathggns commented Nov 3, 2017

Any progress on this?

@Y-WinDow
Copy link

I encounter this issue when I press backspace 2 times.

@elvisvoer
Copy link
Collaborator

@nathggns @Y-WinDow unfortunately this issue has not been addressed or even acknowledged by the maintainers of the project so far. I needed to have react-color in production and the hex8 issue was a total blocker so I ended up creating a fork release with hex8 disabled in react-color.

Until this issue gets resolved you can use the fork release:
"react-color": "https://github.com/elvisvoer/react-color.git#hex8disabled"

@casesandberg
Copy link
Owner

I see the issues you are running into here. I think the best course of action for now would be to invalidate hex4 and hex8. @elvisvoer could you put your fix in a PR, I would be happy to merge it.

@Y-WinDow
Copy link

@elvisvoer thanks a lot!

@elvisvoer
Copy link
Collaborator

@casesandberg submitted PR. Thanks!

@jbub
Copy link

jbub commented Feb 7, 2018

@casesandberg any updates on the PR merge ? seems like its ready to be merged, thanks

@danzaner
Copy link

@casesandberg Thanks for all your work on react-color. I'm sure you are busy, but could we get an update on whether you will merge @elvisvoer 's PR from Jan 12? I'd rather not switch to a fork of react-color, but this bug is going to force me to if the PR is not merged.

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

Successfully merging a pull request may close this issue.

8 participants