-
Notifications
You must be signed in to change notification settings - Fork 3
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
Adjust data source slug input #195
Conversation
Signed-off-by: brookewp <brooke.kaminski@automattic.com>
Signed-off-by: brookewp <brooke.kaminski@automattic.com>
|
||
// eslint-disable-next-line @typescript-eslint/no-misused-promises | ||
const debouncedCheckSlugConflict = useDebounce( checkSlugConflict, 500 ); | ||
const onSlugChange = ( newSlug: string | undefined ): void => { | ||
const onSlugUpdate = () => { |
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.
Theoretically we could make this an uncontrolled text input and use the parameter to this function to get e.target.value
instead of having to update state on every onChange
event. Really no need for the newSlug
useState
at all if we do it that way which reduces rerenders.
const onSlugUpdate = () => { | |
const onSlugUpdate: React.ComponentProps< typeof TextControl >[ 'onBlur' ] = e => { |
However, for some reason, the TextControl
component we're using from @wordpress/components
forces you to have an onChange
and a value
prop... so they seem to really want you to use it as a controlled component.
So maybe just ignore all of this. I'd be interested to know why the value
and onChange
props are required though.
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 definitely improves the UX of typing into the field, but we should go ahead and improve the sanitization to be a little more robust.
With the help of AI, something like this:
function slugify(input: string) {
return input
.toString() // Ensure input is a string
.toLowerCase() // Convert to lowercase
.trim() // Trim leading and trailing spaces
.replace(/\s+/g, '-') // Replace spaces with hyphens
.replace(/[^a-z0-9-]/g, '') // Remove invalid characters
.replace(/--+/g, '-') // Replace multiple hyphens with a single hyphen
.replace(/^-+|-+$/g, ''); // Trim leading and trailing hyphens
}
Signed-off-by: brookewp <brooke.kaminski@automattic.com>
Good point! Replacing spaces with hyphens is a bonus UX improvement too 😄 Implemented that in: |
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.
Looks great. I'm not going to request the change, but I do wonder if we should rename the sanitize function name because it's really not just sanitization, but it's a transformation into slug form (so slugify
makes more sense to me)... but it might just be pedantic and unnecessary.
Signed-off-by: brookewp <brooke.kaminski@automattic.com>
I have no objections. 🙂 I updated that here and added jsdocs to the function to hopefully help clarify it further |
The data source slug can only contain lowercase alphanumeric characters and hyphens, so currently it isn't possible to enter anything else into the input field, including capital letters.
We already use the method
toLowerCase
in thesanitizeDataSourceSlug
function, so we can adjust the regex to be case insensitive to allow for capitals to be entered before sanitizing.This PR also moves the logic from the input's
onChange
toonBlur
, so there is visual feedback showing capitals are being entered, but when leaving the field, it'll be adjusted to lower case:In the future, we may want to consider adding some warning to let the user know these characters are invalid for this field.