-
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
Add Inline Error for Protected Post Password Field and Uncheck "Password Protected" for Empty Field #67034
base: trunk
Are you sure you want to change the base?
Add Inline Error for Protected Post Password Field and Uncheck "Password Protected" for Empty Field #67034
Conversation
… option when password is empty
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @sarthaknagoshe2002! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
Thanks a lot @sarthaknagoshe2002! I tested:
Here's a string of 256 chars to test:
I think it handles the edge cases well too, e.g.,
I'll just add a couple of reviewers for sanity checks. |
@@ -256,8 +273,12 @@ export default function PostStatus() { | |||
id={ passwordInputId } | |||
__next40pxDefaultSize | |||
__nextHasNoMarginBottom | |||
maxLength={ 255 } |
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.
Should we keep this check and instead of an error
show a warning that the pasted code has been trimmed?
Additionally we could also add this limitation to a help text.. 🤔
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.
If we keep that line, this condition won’t be triggered:
if ( newPassword.length > 255 ) {
setError( __( 'Password cannot exceed 255 characters.' ) );
This means we won’t be able to check if the character limit is exceeded. To handle that, we would need to listen for keydown
and paste
events in order to show an error or warning when the limit is breached.
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 mean listening events won't be that straight-forward as this approach.
If your experience says that we should change the approach then lets do it 🚀.
Let me know accordingly 😇
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.
show a warning that the pasted code has been trimmed?
Just to make things clear, the error should be converted to warning & the message should say the following right?
Password cannot exceed 255 characters \n The pasted password may have been trimmed
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.
Yeah, I see the dilemma. It would be nice to keep the HTML attribute for compatibility. HTML validation attributes also work well with assistive technologies, improving accessibility.
There's another, low-tech option that might be better UX: just tell folks that the maximum is 255 before they enter, and reinstate the maxLenth
attr.
Given that it's an edge case anyway, I don't think we need to overthink it.
Then we could just remove the error message and associated handlers.
What do folks think?
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.
Okay, so we'll tell them the limit's 255, and anything extra gets cut off.
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.
It seems a shame to clutter the UI with help text, given this is likely an extreme edge case. That said the input controls have no style support for error handling, so I'd be wary of creating an ad hoc solution for that here.
I'm likely missing a nuance (apologies) but could it make sense to use browser native UI for this? Here's an example.
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'm likely missing a nuance (apologies) but could it make sense to use browser native UI for this? Here's an example.
This seems like a good option, provided it doesn’t impact the validation process.
Another workaround could be displaying a warning at 255 characters, indicating that any additional characters will be trimmed off. This way we can keep the warning & the maxLenth
attr too.
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.
This seems like a good option, provided it doesn’t impact the validation process.
Another workaround could be displaying a warning at 255 characters, indicating that any additional characters will be trimmed off. This way we can keep the warning & the
maxLenth attr
too.
@ramonjd @ntsekouras @jameskoster awaiting your feedbacks on this suggestion.
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.
Worth a try, I think.
Fixes: #64157
What?
This PR addresses two issues related to the post password field:
Why?
Password Protected
checkbox when the password field is cleared ensures that the UI is in sync with the actual post protection state, reducing confusion for users.How?
Password Protected
checkbox, the state is updated to automatically uncheck the box when the password field is cleared. This is done by updating theshowPassword
state when the password field is empty & theDropdown
is out ofFocus
, ensuring the checkbox reflects the correct state.Testing Instructions
Test Inline Error for Password Length:
Password
field.Test
Password Protected
Checkbox Behavior:Password Protected
checkbox and enter a password.Screenshots or screencast
Password.protection.bug.mov