-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Strip double # char on HexInput #38335
Strip double # char on HexInput #38335
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution @KevinBatdorf ! I think this is a pretty reasonable change to make.
Let's check in with @sarayourfriend and @jorgefilipecosta on what the intended validation behavior was meant to be for this field. I noticed while reviewing that we never call the validation function. ColorHexInputControl needs the isPressEnterToChange
property to check values.
const handleChange = ( nextValue ) => { | ||
const hexValue = ( '#' + nextValue ).replace( '##', '#' ); | ||
onChange( colord( hexValue ) ); | ||
}; | ||
const handleValidate = ( value: string ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed this while reviewing that we never call the validation function. ColorHexInputControl needs the isPressEnterToChange
property to check values.
Here's what the behavior looks like when this is set. If invalid values are entered, we keep the last color value. The tradeoff though is that the color value needs an enter keypress, versus updating as we type.
CleanShot.2022-02-11.at.15.30.15.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you able to actually trigger the validation handler? Even with bad input I can't get it to throw the error. The console remains empty.
I updated it to include isPressEnterToChange
and also added an onPaste
handler that will trigger the handleChange
function.
What do you think about that?
If onValidate
isn't working as expected, should we just throw on bad input within handleChange
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh so I don't think we need to change the behavior to add isPressEnterToChange
, I was just noting that the handleValidate
doesn't do anything. Unless other folks chime in, I think it's okay to remove the handleValidate
handler, so folks don't wonder why nothing is happening.
gutenberg/packages/components/src/input-control/input-field.tsx
Lines 160 to 162 in ea32685
if ( isPressEnterToChange ) { | |
event.preventDefault(); | |
handleOnCommit( event ); |
Are you able to actually trigger the validation handler?
Yes, I'm able to. One thing I am running into is that the max length set on the input will truncate the values we are interested in. This appears to be the case for osx / Chrome,FF,Safari combos. Which os/browser are you using? I wonder if we're seeing some subtle browser differences.
paste.validate.behavior.mp4
I think we're pretty close. Playing around with this, it gets a little tricky if we start incorporating the onPaste handler. If we're trying to modify paste events, we normally preventDefault, then manually update the input. What about the following?
It should look like this all together: CleanShot.2022-02-14.at.09.36.08.mp4The only slightly finicky thing is that we show another cc @jasmussen for the proposed behavior changes, of max input length from 6 to 7 |
Appreciate the ping, thank you. Judging only by the video, this seems like a totally fine sanitization to me. In fact I often find myself frustrated with inputs that truncate hex codes due to the addition of the hash mark ⭐ |
I'm not inside of the intended validation behavior but the plan @gwwar suggested seems good to me (provided in max length we handle the case where alpha is enabled). |
Excellent, sounds like we have a plan. Once we have changes on the branch, it should be pretty quick to retest, approve, and merge. @KevinBatdorf let me know if anything is unclear, or if you'd prefer for me to add changes directly to the branch. |
Hi everyone. Thanks for the feedback. I'll most likely follow up on this tomorrow or if not tomorrow then definitely Friday (we are doing our FFTF this Friday!). |
Hi @gwwar I added these changes:
I also updated the max length for when
Feel free. I read the messages in a rush the other day so missed this part, but overall it's always okay with me if someone wants to take over a branch here. I'm trying to jump in during my free time so I don't yet have a full understanding of all the components and how they interact with each other. If you want to make some more changes feel free! Thanks for the feedback and everything else as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @KevinBatdorf for the contribution! This tests very well for me. Here's what I see:
copy.and.paste.with.mp4
I'll merge this for you once CI tests are ✅
Thanks @gwwar One optimization I think would make it even better is if it would auto focus the input when you press the control button. I may explore that later on. |
Description
This strips out the leading
#
when setting a color hex value on the editorCloses #38334
Testing Instructions
Screenshots
Check the issue linked above for a screencast of the previous behavior
hex.paste.updated.mov
Types of changes
Bug fix
Checklist:
*.native.js
files for terms that need renaming or removal).