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: displayName regex #6138

Merged
merged 2 commits into from
Jun 23, 2023
Merged

fix: displayName regex #6138

merged 2 commits into from
Jun 23, 2023

Conversation

nikk15
Copy link
Contributor

@nikk15 nikk15 commented Jun 21, 2023

Solves this console error:
Pattern attribute value ^[A-Za-z0-9_~ -]{3,32}$ is not a valid regular expression: Uncaught SyntaxError: Invalid regular expression: /^[A-Za-z0-9_~ -]{3,32}$/v: Invalid character in character class

Copy link
Contributor

@keianhzo keianhzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that browsers are a bit picky and each one them have their on wierdness so I've tried my best to find the right solution. I've left a comment about my findings.

Comment on lines 66 to 68
displayName: { type: "string", pattern: "^(?:[A-Z]|[a-z]|[0-9]|[_~\\-]|\\s){3,32}$" },
avatarId: { type: "string" },
pronouns: { type: "string", pattern: "^[a-zA-Z///a-zA-Z]{2,32}$" },
Copy link
Contributor

@keianhzo keianhzo Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to still make Chrome complain. After some testing this seems not to raise any complaints in both Chrome & Firefox:

displayName: { type: "string", pattern: "^[A-Za-z0-9_~\\s\\-]{3,32}$" },
avatarId: { type: "string" },
pronouns: { type: "string", pattern: "^([a-zA-Z]{1,32}\\/){0,4}[a-zA-Z]{1,32}$" },

This pronouns regex is a bit more strict and enforces the A/B form and you can have up to 5 pronouns that I think it's probably enough for what I've seen?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we prefer to keep the previous regex format, this would work:

^[a-zA-Z\\/a-zA-Z]{2,32}$

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! Thanks for looking into alternatives. I'll update the pronouns with your suggestion - its currently had a few incarnations (enforcing one '/', and leaving it out as an option) but I like your suggestion that allows for up to 5 pronouns. 🚀

@nikk15 nikk15 temporarily deployed to smoke June 22, 2023 15:10 — with GitHub Actions Inactive
@nikk15 nikk15 temporarily deployed to hc-bio June 22, 2023 15:10 — with GitHub Actions Inactive
@keianhzo keianhzo self-requested a review June 23, 2023 09:46
Copy link
Contributor

@keianhzo keianhzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

@nikk15 nikk15 merged commit f62c7ab into master Jun 23, 2023
12 checks passed
@nikk15 nikk15 deleted the pronoun-regex-fix branch June 23, 2023 19:42
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 this pull request may close these issues.

None yet

2 participants