-
Notifications
You must be signed in to change notification settings - Fork 24
feat(ui): Add PhoneInput component with international formatting support #408
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,235 @@ | ||||
| import * as React from 'react'; | ||||
| import { parsePhoneNumber, AsYouType, CountryCode } from 'libphonenumber-js'; | ||||
| import { Input, InputWithLabel, InputWithDetails, InputProps, InputWithDetailsProps } from './input'; | ||||
| import { cn } from '@o2s/ui/lib/utils'; | ||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove unused import. The Apply this diff: -import { cn } from '@o2s/ui/lib/utils';📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||
|
|
||||
| // Minimal metadata for supported countries (EN, DE, PL) | ||||
| const SUPPORTED_COUNTRIES: CountryCode[] = ['GB', 'DE', 'PL']; | ||||
|
Comment on lines
+6
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove or utilize the unused constant. The 🤖 Prompt for AI Agents |
||||
|
|
||||
| export type PhoneInputProps = Omit<InputProps, 'onChange' | 'value'> & { | ||||
| defaultCountry?: CountryCode; | ||||
| value?: string; | ||||
| onChange?: (value: string, isValid: boolean) => void; | ||||
| onValidationChange?: (isValid: boolean) => void; | ||||
| }; | ||||
|
|
||||
| export type PhoneInputOwnProps = PhoneInputProps & { ref?: React.Ref<HTMLInputElement> }; | ||||
|
|
||||
| const PhoneInput = React.forwardRef<HTMLInputElement, PhoneInputOwnProps>( | ||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please add a Storybook file for this component so it can actually be tested, once it's done I will check how it works |
||||
| ({ defaultCountry = 'PL', value = '', onChange, onValidationChange, ...props }, ref) => { | ||||
| const [displayValue, setDisplayValue] = React.useState(value); | ||||
| const [isValid, setIsValid] = React.useState(false); | ||||
|
|
||||
| React.useEffect(() => { | ||||
| setDisplayValue(value); | ||||
| }, [value]); | ||||
|
|
||||
| const formatPhoneNumber = React.useCallback( | ||||
| (input: string) => { | ||||
| if (!input) return ''; | ||||
|
|
||||
| try { | ||||
| // Try to parse the phone number | ||||
| const asYouType = new AsYouType(defaultCountry); | ||||
| const formatted = asYouType.input(input); | ||||
|
|
||||
| // Check if it's valid | ||||
| const phoneNumber = asYouType.getNumber(); | ||||
| const valid = phoneNumber ? phoneNumber.isValid() : false; | ||||
|
|
||||
| setIsValid(valid); | ||||
| onValidationChange?.(valid); | ||||
|
|
||||
| return formatted; | ||||
| } catch (error) { | ||||
| setIsValid(false); | ||||
| onValidationChange?.(false); | ||||
| return input; | ||||
| } | ||||
| }, | ||||
| [defaultCountry, onValidationChange] | ||||
| ); | ||||
|
|
||||
| const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||||
| const inputValue = e.target.value; | ||||
|
|
||||
| // Only allow valid characters (numbers, +, spaces, parentheses, hyphens) | ||||
| const sanitized = inputValue.replace(/[^0-9+\s()-]/g, ''); | ||||
|
|
||||
| const formatted = formatPhoneNumber(sanitized); | ||||
| setDisplayValue(formatted); | ||||
| onChange?.(formatted, isValid); | ||||
| }; | ||||
|
Comment on lines
+53
to
+62
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Race condition in validation status. The Example:
This affects all three components in this file. Solution: Return the validation status from const formatPhoneNumber = React.useCallback(
(input: string) => {
if (!input) return '';
try {
const asYouType = new AsYouType(defaultCountry);
const formatted = asYouType.input(input);
const phoneNumber = asYouType.getNumber();
const valid = phoneNumber ? phoneNumber.isValid() : false;
setIsValid(valid);
onValidationChange?.(valid);
- return formatted;
+ return { formatted, valid };
} catch (error) {
setIsValid(false);
onValidationChange?.(false);
- return input;
+ return { formatted: input, valid: false };
}
},
[defaultCountry, onValidationChange]
);
const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => {
const inputValue = e.target.value;
const sanitized = inputValue.replace(/[^0-9+\s()-]/g, '');
- const formatted = formatPhoneNumber(sanitized);
+ const { formatted, valid } = formatPhoneNumber(sanitized);
setDisplayValue(formatted);
- onChange?.(formatted, isValid);
+ onChange?.(formatted, valid);
};Apply the same fix to
🤖 Prompt for AI Agents |
||||
|
|
||||
| const handlePaste = (e: React.ClipboardEvent<HTMLInputElement>) => { | ||||
| e.preventDefault(); | ||||
| const pastedText = e.clipboardData.getData('text'); | ||||
| const sanitized = pastedText.replace(/[^0-9+\s()-]/g, ''); | ||||
| const formatted = formatPhoneNumber(sanitized); | ||||
| setDisplayValue(formatted); | ||||
| onChange?.(formatted, isValid); | ||||
| }; | ||||
|
|
||||
| return ( | ||||
| <Input | ||||
| {...props} | ||||
| ref={ref} | ||||
| type="tel" | ||||
| value={displayValue} | ||||
| onChange={handleChange} | ||||
| onPaste={handlePaste} | ||||
| placeholder="+48 500 500 500" | ||||
| /> | ||||
| ); | ||||
| } | ||||
| ); | ||||
|
|
||||
| PhoneInput.displayName = 'PhoneInput'; | ||||
|
|
||||
| export type PhoneInputWithLabelProps = Omit<InputWithDetailsProps, 'onChange' | 'value'> & { | ||||
| defaultCountry?: CountryCode; | ||||
| value?: string; | ||||
| onChange?: (value: string, isValid: boolean) => void; | ||||
| onValidationChange?: (isValid: boolean) => void; | ||||
| }; | ||||
|
Comment on lines
+89
to
+94
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incorrect base type for PhoneInputWithLabel.
According to the relevant code snippets, -export type PhoneInputWithLabelProps = Omit<InputWithDetailsProps, 'onChange' | 'value'> & {
+export type PhoneInputWithLabelProps = Omit<InputWithLabelProps, 'onChange' | 'value'> & {
defaultCountry?: CountryCode;
value?: string;
onChange?: (value: string, isValid: boolean) => void;
onValidationChange?: (isValid: boolean) => void;
};You'll also need to import
🤖 Prompt for AI Agents |
||||
|
|
||||
| const PhoneInputWithLabel = React.forwardRef<HTMLInputElement, PhoneInputWithLabelProps>( | ||||
| ({ defaultCountry = 'PL', value = '', onChange, onValidationChange, ...props }, ref) => { | ||||
| const [displayValue, setDisplayValue] = React.useState(value); | ||||
| const [isValid, setIsValid] = React.useState(false); | ||||
|
|
||||
| React.useEffect(() => { | ||||
| setDisplayValue(value); | ||||
| }, [value]); | ||||
|
|
||||
| const formatPhoneNumber = React.useCallback( | ||||
| (input: string) => { | ||||
| if (!input) return ''; | ||||
|
|
||||
| try { | ||||
| const asYouType = new AsYouType(defaultCountry); | ||||
| const formatted = asYouType.input(input); | ||||
| const phoneNumber = asYouType.getNumber(); | ||||
| const valid = phoneNumber ? phoneNumber.isValid() : false; | ||||
|
|
||||
| setIsValid(valid); | ||||
| onValidationChange?.(valid); | ||||
|
|
||||
| return formatted; | ||||
| } catch (error) { | ||||
| setIsValid(false); | ||||
| onValidationChange?.(false); | ||||
| return input; | ||||
| } | ||||
| }, | ||||
| [defaultCountry, onValidationChange] | ||||
| ); | ||||
|
|
||||
| const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||||
| const inputValue = e.target.value; | ||||
| const sanitized = inputValue.replace(/[^0-9+\s()-]/g, ''); | ||||
| const formatted = formatPhoneNumber(sanitized); | ||||
| setDisplayValue(formatted); | ||||
| onChange?.(formatted, isValid); | ||||
| }; | ||||
|
|
||||
| const handlePaste = (e: React.ClipboardEvent<HTMLInputElement>) => { | ||||
| e.preventDefault(); | ||||
| const pastedText = e.clipboardData.getData('text'); | ||||
| const sanitized = pastedText.replace(/[^0-9+\s()-]/g, ''); | ||||
| const formatted = formatPhoneNumber(sanitized); | ||||
| setDisplayValue(formatted); | ||||
| onChange?.(formatted, isValid); | ||||
| }; | ||||
|
|
||||
| return ( | ||||
| <InputWithLabel | ||||
| {...props} | ||||
| ref={ref} | ||||
| type="tel" | ||||
| value={displayValue} | ||||
| onChange={handleChange} | ||||
| onPaste={handlePaste} | ||||
| placeholder="+48 500 500 500" | ||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. placeholder CAN NOT be hardcoded |
||||
| /> | ||||
| ); | ||||
| } | ||||
| ); | ||||
|
|
||||
| PhoneInputWithLabel.displayName = 'PhoneInputWithLabel'; | ||||
|
|
||||
| export type PhoneInputWithDetailsProps = Omit<InputWithDetailsProps, 'onChange' | 'value'> & { | ||||
| defaultCountry?: CountryCode; | ||||
| value?: string; | ||||
| onChange?: (value: string, isValid: boolean) => void; | ||||
| onValidationChange?: (isValid: boolean) => void; | ||||
| }; | ||||
|
|
||||
| const PhoneInputWithDetails = React.forwardRef<HTMLInputElement, PhoneInputWithDetailsProps>( | ||||
| ({ defaultCountry = 'PL', value = '', onChange, onValidationChange, caption, errorMessage, ...props }, ref) => { | ||||
| const [displayValue, setDisplayValue] = React.useState(value); | ||||
| const [isValid, setIsValid] = React.useState(false); | ||||
|
|
||||
| React.useEffect(() => { | ||||
| setDisplayValue(value); | ||||
| }, [value]); | ||||
|
|
||||
| const formatPhoneNumber = React.useCallback( | ||||
| (input: string) => { | ||||
| if (!input) return ''; | ||||
|
|
||||
| try { | ||||
| const asYouType = new AsYouType(defaultCountry); | ||||
| const formatted = asYouType.input(input); | ||||
| const phoneNumber = asYouType.getNumber(); | ||||
| const valid = phoneNumber ? phoneNumber.isValid() : false; | ||||
|
|
||||
| setIsValid(valid); | ||||
| onValidationChange?.(valid); | ||||
|
|
||||
| return formatted; | ||||
| } catch (error) { | ||||
| setIsValid(false); | ||||
| onValidationChange?.(false); | ||||
| return input; | ||||
| } | ||||
| }, | ||||
| [defaultCountry, onValidationChange] | ||||
| ); | ||||
|
|
||||
| const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||||
| const inputValue = e.target.value; | ||||
| const sanitized = inputValue.replace(/[^0-9+\s()-]/g, ''); | ||||
| const formatted = formatPhoneNumber(sanitized); | ||||
| setDisplayValue(formatted); | ||||
| onChange?.(formatted, isValid); | ||||
| }; | ||||
|
|
||||
| const handlePaste = (e: React.ClipboardEvent<HTMLInputElement>) => { | ||||
| e.preventDefault(); | ||||
| const pastedText = e.clipboardData.getData('text'); | ||||
| const sanitized = pastedText.replace(/[^0-9+\s()-]/g, ''); | ||||
| const formatted = formatPhoneNumber(sanitized); | ||||
| setDisplayValue(formatted); | ||||
| onChange?.(formatted, isValid); | ||||
| }; | ||||
|
|
||||
| return ( | ||||
| <InputWithDetails | ||||
| {...props} | ||||
| ref={ref} | ||||
| type="tel" | ||||
| value={displayValue} | ||||
| onChange={handleChange} | ||||
| onPaste={handlePaste} | ||||
| placeholder="+48 500 500 500" | ||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. again, please remove hardcoded placeholder |
||||
| caption={caption} | ||||
| errorMessage={errorMessage} | ||||
| /> | ||||
| ); | ||||
| } | ||||
| ); | ||||
|
|
||||
| PhoneInputWithDetails.displayName = 'PhoneInputWithDetails'; | ||||
|
Comment on lines
+18
to
+233
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Extract shared logic to eliminate code duplication. All three components (
This violates DRY principles and triples the maintenance burden. Recommended approach: Extract the shared logic into a custom hook: function usePhoneInput(
value: string,
defaultCountry: CountryCode,
onChange?: (value: string, isValid: boolean) => void,
onValidationChange?: (isValid: boolean) => void
) {
const [displayValue, setDisplayValue] = React.useState(value);
const [isValid, setIsValid] = React.useState(false);
React.useEffect(() => {
setDisplayValue(value);
}, [value]);
const formatPhoneNumber = React.useCallback(
(input: string) => {
if (!input) return { formatted: '', valid: false };
try {
const asYouType = new AsYouType(defaultCountry);
const formatted = asYouType.input(input);
const phoneNumber = asYouType.getNumber();
const valid = phoneNumber ? phoneNumber.isValid() : false;
setIsValid(valid);
onValidationChange?.(valid);
return { formatted, valid };
} catch (error) {
setIsValid(false);
onValidationChange?.(false);
return { formatted: input, valid: false };
}
},
[defaultCountry, onValidationChange]
);
const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => {
const inputValue = e.target.value;
const sanitized = inputValue.replace(/[^0-9+\s()-]/g, '');
const { formatted, valid } = formatPhoneNumber(sanitized);
setDisplayValue(formatted);
onChange?.(formatted, valid);
};
const handlePaste = (e: React.ClipboardEvent<HTMLInputElement>) => {
e.preventDefault();
const pastedText = e.clipboardData.getData('text');
const sanitized = pastedText.replace(/[^0-9+\s()-]/g, '');
const { formatted, valid } = formatPhoneNumber(sanitized);
setDisplayValue(formatted);
onChange?.(formatted, valid);
};
return {
displayValue,
isValid,
handleChange,
handlePaste,
};
}Then simplify each component: const PhoneInput = React.forwardRef<HTMLInputElement, PhoneInputOwnProps>(
({ defaultCountry = 'PL', value = '', onChange, onValidationChange, ...props }, ref) => {
const { displayValue, handleChange, handlePaste } = usePhoneInput(
value,
defaultCountry,
onChange,
onValidationChange
);
return (
<Input
{...props}
ref={ref}
type="tel"
value={displayValue}
onChange={handleChange}
onPaste={handlePaste}
placeholder="+48 500 500 500"
/>
);
}
);🤖 Prompt for AI Agents |
||||
|
|
||||
| export { PhoneInput, PhoneInputWithLabel, PhoneInputWithDetails }; | ||||
Uh oh!
There was an error while loading. Please reload this page.
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.
Remove unused import.
The
parsePhoneNumberimport is never used in this file.Apply this diff:
📝 Committable suggestion
🤖 Prompt for AI Agents
🧩 Analysis chain
Verify bundle size impact and metadata approach.
The PR objectives and issue #306 specifically requested minimizing bundle size by using
MetadataJsonwith metadata for only the supported countries (GB, DE, PL) instead of full metadata. However, the current implementation imports fromlibphonenumber-js, which includes metadata for all 200+ countries.Please verify whether:
libphonenumber-js/minorlibphonenumber-js/corewith custom metadata as described in the libphonenumber-js documentation.Run the following script to check the current bundle size impact:
🏁 Script executed:
Length of output: 273
🏁 Script executed:
Length of output: 513
🏁 Script executed:
Length of output: 180
🏁 Script executed:
Length of output: 3567
🏁 Script executed:
Length of output: 7794
Add
libphonenumber-jsdependency topackages/ui/package.jsonand implement custom metadata to minimize bundle size.Two critical issues were found:
Missing dependency declaration:
libphonenumber-jsis imported but not declared inpackages/ui/package.json. In this monorepo workspace structure, the package must explicitly declare its dependencies.Bundle size not optimized: The code includes a comment about "Minimal metadata for supported countries" and defines
SUPPORTED_COUNTRIESfor only 3 countries (GB, DE, PL), but the implementation uses the fulllibphonenumber-jslibrary which bundles metadata for 200+ countries. To minimize bundle size as intended, implement custom metadata by usinglibphonenumber-js/minwith a custom metadata file containing only the required countries' data, as documented in the libphonenumber-js documentation.Required changes:
"libphonenumber-js": "<version>"topackages/ui/package.jsondependencieslibphonenumber-js/minand provide custom metadataThere 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.
@mithilP007 yes, please optimize loaded metadata so that not all countries are imported
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.