feat: phone number fields#31
Merged
Merged
Conversation
…ration * Updated form_field.json and form_field.py to include 'Phone' in the fieldtype options. * Adjusted the modified timestamp in the JSON file.
* Added Phone.vue component for handling phone number input with country selection. * Updated form field types to include Phone as a selectable option. * Introduced PhoneField in form_fields.ts to integrate the new component into the form builder.
…r handling * Added phone number sanitization to convert spaces to dashes and remove dashes from the number part for model storage. * Updated display logic to format phone numbers with spaces for user interface. * Improved visibility logic in FormRenderer by ensuring consistent parameter handling in computed properties.
📝 WalkthroughWalkthroughThis PR adds support for a Phone field type across the application stack. It extends the backend form field doctype to include "Phone", updates frontend type definitions, and introduces a new Phone.vue component with country selection and phone number formatting capabilities. Changes
Possibly Related Issues
Possibly Related PRs
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
forms_pro/forms_pro/doctype/form_field/form_field.py (1)
21-39: Add "Phone" handler toto_frappe_fieldmethod.Frappe does not recognize "Phone" as a native fieldtype. Line 38 adds "Phone" to the fieldtype literal, but the
to_frappe_fieldmethod lacks a handler for it. This will cause runtime errors when creating forms with phone fields.Add a case mapping "Phone" to Data with options="Phone", following the existing pattern used for Email (lines 53-55):
Suggested fix
elif self.fieldtype == "Phone": _fieldtype = "Data" self.options = "Phone"Add after line 55 in the
to_frappe_fieldmethod.
🤖 Fix all issues with AI agents
In `@frontend/src/components/fields/Phone.vue`:
- Around line 135-147: sanitizePhoneNumber currently only removes dashes from
the number part, leaving internal spaces (e.g., "+91-081234 56789"); update
sanitizePhoneNumber to also strip all whitespace from the numberPart (in
addition to dashes) before returning the canonical form, so the function (which
matches countryCode via /^(\+\d+)[\s-]+(.+)$/) returns countryCode + "-" +
cleanedNumber with all spaces and dashes removed and otherwise preserves the
existing fallback behavior.
- Around line 233-281: The country dropdown uses non-focusable divs so keyboard
users cannot navigate or select options; update the dropdown
(countryDropdownRef) to use proper listbox semantics: add role="listbox" on the
container and role="option" on each item, make each option focusable
(tabindex="0"), handle keyboard events on options (Enter/Space to call
handleCountryChange(option.value) and Close dropdown by setting
isCountryDropdownOpen = false), and manage aria-selected based on
selectedCountryCode; also ensure countryButtonRef has aria-expanded and
aria-controls pointing at the listbox id and that focus moves into the first
option when opening for keyboard navigation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
forms_pro/forms_pro/doctype/form_field/form_field.jsonforms_pro/forms_pro/doctype/form_field/form_field.pyfrontend/src/components/fields/Phone.vuefrontend/src/types/formfield.tsfrontend/src/utils/form_fields.ts
🧰 Additional context used
🪛 Ruff (0.14.11)
forms_pro/forms_pro/doctype/form_field/form_field.py
38-38: Undefined name Phone
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Server
🔇 Additional comments (3)
forms_pro/forms_pro/doctype/form_field/form_field.json (1)
40-40: LGTM — "Phone" added to selectable field types.frontend/src/types/formfield.ts (1)
12-12: LGTM — enum extended for Phone.frontend/src/utils/form_fields.ts (1)
17-18: LGTM — Phone field registered and wired.Also applies to: 140-165
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
+135
to
+147
| // Sanitize phone number: convert space to dash and remove dashes from number part (for model storage) | ||
| const sanitizePhoneNumber = (phoneNumber: string): string => { | ||
| if (!phoneNumber) return ""; | ||
| // Match country code and number parts (handles both space and dash separators) | ||
| const match = phoneNumber.match(/^(\+\d+)[\s-]+(.+)$/); | ||
| if (match) { | ||
| const countryCode = match[1]; | ||
| const numberPart = match[2].replace(/-/g, ""); // Remove all dashes from number part | ||
| // Return as "+91-1234567890" (dash between country code and number, no dashes in number) | ||
| return `${countryCode}-${numberPart}`; | ||
| } | ||
| // Fallback: if format doesn't match, return as-is (shouldn't happen in normal flow) | ||
| return phoneNumber; |
There was a problem hiding this comment.
Normalize stored phone numbers consistently.
sanitizePhoneNumber only strips dashes, so spaces inside the number part remain (e.g., “+91-081234 56789”), which contradicts the intent of “sanitize.” Consider stripping whitespace too to store a stable canonical form. Lines 135‑145.
🧹 Proposed fix
- const numberPart = match[2].replace(/-/g, ""); // Remove all dashes from number part
+ const numberPart = match[2].replace(/[\s-]/g, ""); // Remove spaces and dashes from number part🤖 Prompt for AI Agents
In `@frontend/src/components/fields/Phone.vue` around lines 135 - 147,
sanitizePhoneNumber currently only removes dashes from the number part, leaving
internal spaces (e.g., "+91-081234 56789"); update sanitizePhoneNumber to also
strip all whitespace from the numberPart (in addition to dashes) before
returning the canonical form, so the function (which matches countryCode via
/^(\+\d+)[\s-]+(.+)$/) returns countryCode + "-" + cleanedNumber with all spaces
and dashes removed and otherwise preserves the existing fallback behavior.
Comment on lines
+233
to
+281
| <div class="relative flex items-center h-full"> | ||
| <button | ||
| ref="countryButtonRef" | ||
| type="button" | ||
| @click.stop="isCountryDropdownOpen = !isCountryDropdownOpen" | ||
| class="flex items-center gap-1 px-2 h-full hover:bg-gray-100 focus:outline-none focus:bg-gray-100 whitespace-nowrap border-r border-gray-300 pr-2 mr-2 min-w-[85px]" | ||
| > | ||
| <span class="text-base leading-none flex-shrink-0">{{ | ||
| selectedCountry?.flag | ||
| }}</span> | ||
| <span | ||
| class="text-sm font-medium text-gray-700 leading-none flex-shrink-0" | ||
| >{{ selectedCountry?.dialCode }}</span | ||
| > | ||
| <div class="flex flex-col ml-auto flex-shrink-0"> | ||
| <ChevronUp class="w-3 h-3 text-gray-500" /> | ||
| <ChevronDown class="w-3 h-3 text-gray-500 -mt-1" /> | ||
| </div> | ||
| </button> | ||
|
|
||
| <!-- Dropdown Menu --> | ||
| <div | ||
| v-if="isCountryDropdownOpen" | ||
| ref="countryDropdownRef" | ||
| class="absolute top-full left-0 mt-1 w-64 max-h-60 overflow-y-auto bg-white border border-gray-300 rounded-md shadow-lg z-50" | ||
| @click.stop | ||
| > | ||
| <div class="p-1"> | ||
| <div | ||
| v-for="option in countryOptions" | ||
| :key="option.value" | ||
| @click=" | ||
| handleCountryChange(option.value); | ||
| isCountryDropdownOpen = false; | ||
| " | ||
| class="flex items-center gap-2 px-3 py-2 cursor-pointer hover:bg-gray-100 rounded" | ||
| :class="{ | ||
| 'bg-blue-50': selectedCountryCode === option.value, | ||
| }" | ||
| > | ||
| <span class="text-lg">{{ option.country.flag }}</span> | ||
| <span class="text-sm font-medium text-gray-700">{{ | ||
| option.country.dialCode | ||
| }}</span> | ||
| <span class="text-sm text-gray-600 ml-auto">{{ | ||
| option.country.name | ||
| }}</span> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Dropdown options aren’t keyboard-accessible.
The country menu uses non-focusable <div> items, so keyboard-only users can’t change country—an accessibility blocker. Consider listbox semantics and focusable options. Lines 233‑281.
♿ Proposed fix (minimal listbox semantics)
- <button
+ <button
ref="countryButtonRef"
type="button"
`@click.stop`="isCountryDropdownOpen = !isCountryDropdownOpen"
+ aria-haspopup="listbox"
+ :aria-expanded="isCountryDropdownOpen"
+ aria-controls="phone-country-listbox"
class="flex items-center gap-1 px-2 h-full hover:bg-gray-100 focus:outline-none focus:bg-gray-100 whitespace-nowrap border-r border-gray-300 pr-2 mr-2 min-w-[85px]"
>
@@
- <div
+ <div
v-if="isCountryDropdownOpen"
ref="countryDropdownRef"
+ id="phone-country-listbox"
+ role="listbox"
class="absolute top-full left-0 mt-1 w-64 max-h-60 overflow-y-auto bg-white border border-gray-300 rounded-md shadow-lg z-50"
`@click.stop`
>
<div class="p-1">
- <div
+ <button
v-for="option in countryOptions"
:key="option.value"
- `@click`="
- handleCountryChange(option.value);
- isCountryDropdownOpen = false;
- "
+ type="button"
+ role="option"
+ :aria-selected="selectedCountryCode === option.value"
+ tabindex="0"
+ `@click`="handleCountryChange(option.value); isCountryDropdownOpen = false;"
+ `@keydown.enter.prevent`="handleCountryChange(option.value); isCountryDropdownOpen = false;"
+ `@keydown.space.prevent`="handleCountryChange(option.value); isCountryDropdownOpen = false;"
class="flex items-center gap-2 px-3 py-2 cursor-pointer hover:bg-gray-100 rounded"
:class="{
'bg-blue-50': selectedCountryCode === option.value,
}"
>
@@
- </div>
+ </button>
</div>
</div>🤖 Prompt for AI Agents
In `@frontend/src/components/fields/Phone.vue` around lines 233 - 281, The country
dropdown uses non-focusable divs so keyboard users cannot navigate or select
options; update the dropdown (countryDropdownRef) to use proper listbox
semantics: add role="listbox" on the container and role="option" on each item,
make each option focusable (tabindex="0"), handle keyboard events on options
(Enter/Space to call handleCountryChange(option.value) and Close dropdown by
setting isCountryDropdownOpen = false), and manage aria-selected based on
selectedCountryCode; also ensure countryButtonRef has aria-expanded and
aria-controls pointing at the listbox id and that focus moves into the first
option when opening for keyboard navigation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In form submission page:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.