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

Fix: issue 718, improve validation message for user profile name edit #758

Closed
wants to merge 3 commits into from

Conversation

danekshea
Copy link
Contributor

Quick fix for #718 with an added description of the regex. Didn't test the build due to not having auth available, might be overflow due to length of message but the responsive UI probably handles it find.

@vercel
Copy link

vercel bot commented Mar 26, 2023

@danekshea is attempting to deploy a commit to the openbeta-dev Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Mar 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
open-tacos ✅ Ready (Inspect) Visit Preview Mar 30, 2023 at 6:29PM (UTC)

@vnugent
Copy link
Contributor

vnugent commented Mar 27, 2023

can you please smoke test the change using the preview build? https://open-tacos-aa73c08rd-openbeta-dev.vercel.app/

thanks!

@vnugent
Copy link
Contributor

vnugent commented Apr 7, 2023

Screenshot 2023-04-07 at 5 54 32 AM

you're right - there's not enough room for the long error message.

@zichongkao
Copy link
Contributor

zichongkao commented Apr 28, 2023

I looked into this briefly to find a path forward.

I see several options:

  1. Change the error message display to accommodate larger messages. We could move to a toast, or a tool-tip like modal that is displayed on top of the page content. It could perhaps auto-disappear after several seconds or once the error is fixed.
  2. Break up checkUsername (
    export const checkUsername = (uid: string): boolean => {
    ) into its three constituent subrules:
  • "First and last characters have to be alphanumeric",
  • "User name can only consist of alphanumeric and special characters: '.', '-', '_'"
  • "At most one special character is permitted"
    each with its own check and dedicated error message.
  1. Try a shorter error message: 'Username must be alphanumeric, optionally separated by a ".", "-" or "_".'
  2. Simplify the checkUsername constraints so that it is more easily explained. Eg. "All alphanumeric only", or "Alphanumeric and special characters (., /, -)" + "special characters not permitted in first and last" (ie remove the "at most one special char" rule)

I'd favor 3 and 4. I feel like the current logic is somewhat hard to explain and fairly arbitrary. Perhaps "Only alphanumeric characters and ".", "_", "-" are permitted" would do.

@zichongkao
Copy link
Contributor

@vnugent @danekshea What do y'all think?

@vnugent
Copy link
Contributor

vnugent commented Apr 28, 2023

I'd be in favor of 3 and/or 4. Like you said we can break down the validation into multiple tests, each showing their own shorter message.

@danekshea
Copy link
Contributor Author

3 and 4 sound pretty logical @zichongkao !

@vnugent vnugent added the do not merge Do not merge or approve the pull request label Apr 29, 2023
@vnugent vnugent marked this pull request as draft April 29, 2023 13:12
@vnugent
Copy link
Contributor

vnugent commented May 26, 2023

superseded by #838

@vnugent vnugent closed this May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Do not merge or approve the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants