-
Notifications
You must be signed in to change notification settings - Fork 15
Fix phone number regexes #767
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
Conversation
📝 WalkthroughWalkthroughThe changes correct the HTML input pattern for phone-number validation in two Blade templates by escaping special characters in the regular expressions. A minor whitespace adjustment was also made in the registration view. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
resources/views/user/personal-information.blade.php (1)
80-80: Pattern looks sound – consider complementary UX attributesThe corrected regex now conforms to HTML
patternsyntax – splendid work. For a slightly more polished user experience you might also add atitle,inputmode="tel"orautocomplete="tel"attribute so browsers can display helpful hints and the numeric keypad on mobiles.pattern="\+[0-9]{1,4}[\-\s\(\)0-9]*" +title="+36 (20) 123-4567" +inputmode="tel" +autocomplete="tel"Kindly confirm that the server-side validator accepts the same format, lest client and server drift out of sync.
resources/views/auth/register.blade.php (1)
43-43: Regex correction mirrored correctly; same UX niceties applyThe escaped plus and hyphen resolve the browser warning – good show. As in the personal-information form, consider appending
title,inputmode, andautocompleteattributes for consistency and user guidance.pattern="\+[0-9]{1,4}[\-\s\(\)0-9]*" +title="+36 (20) 123-4567" +inputmode="tel" +autocomplete="tel"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/views/auth/register.blade.php(2 hunks)resources/views/user/personal-information.blade.php(1 hunks)
🔇 Additional comments (2)
resources/views/user/personal-information.blade.php (1)
23-24: No functional change – nothing further to mention.resources/views/auth/register.blade.php (1)
23-23: Whitespace-only adjustment – perfectly acceptable.
|
I'd only fix this after this year's application period as it would be quite confusing for applicants if they edit some other personal information and the system just starts randomly asking them to edit their phone number. There is only one phone number that is actually unusable for non-verified users. I would probably go through manually on the users' data - many start with 06 or 07 - and fix it for them after this PR is merged (but I would only do it after the application period). |
HTML treats input validation patterns as v-mode RegExes, which requires
special characters to be escaped in more cases. The previous patterns
were syntactically invalid, and thus ignored by browsers:
```
Pattern attribute value [+][0-9]{1,4}[-\s()0-9]* is not a valid regular
expression:
Uncaught SyntaxError: Invalid regular expression:
/[+][0-9]{1,4}[-\s()0-9]*/v: Invalid character in character class
```
25c2bd4 to
c97b56e
Compare
HTML treats input validation patterns as v-mode RegExes, which requires special characters to be escaped in more cases. The previous patterns were syntactically invalid, and thus ignored by browsers: