feat: phone input autocomplete#3624
Open
nicosommi wants to merge 9 commits into
Open
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Size Change: +829 B (+0.12%) Total Size: 676 kB 📦 View Changed
ℹ️ View Unchanged
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #3624 +/- ##
==========================================
- Coverage 93.69% 93.68% -0.01%
==========================================
Files 199 199
Lines 4486 4531 +45
Branches 1768 1779 +11
==========================================
+ Hits 4203 4245 +42
- Misses 256 259 +3
Partials 27 27
🚀 New features to boost your workflow:
|
Contributor
|
Thank you for your contribution, Nico ! |
Member
Author
@sirineJ I will prepare it for review without the variant 🙌 |
sirineJ
reviewed
May 28, 2026
| renderPrefix={Prefix} | ||
| />, | ||
| ); | ||
| expect(screen.getByTestId('prefix')).toHaveTextContent('CA'); |
Contributor
There was a problem hiding this comment.
Prefer getByText() over getByTestId() to better mimic users' perception of the UI
Suggested change
| expect(screen.getByTestId('prefix')).toHaveTextContent('CA'); | |
| expect(screen.getByText('CA')).toBeVisible(); |
| position: absolute; | ||
| top: 50%; | ||
| left: 0; | ||
| z-index: calc(var(--cui-z-index-input) + 1); |
Contributor
There was a problem hiding this comment.
why assign a z-index value ?
Member
Author
There was a problem hiding this comment.
There was a border conflict before when having the Select, removed now 👍
| defaultValue={defaultValue} | ||
| {...props} | ||
| /> | ||
| {readOnly || countryCode.readonly ? ( |
Contributor
There was a problem hiding this comment.
Do we still need this readOnly Input now that we have the AutocompleteInput that also supports a read only state ?
Member
Author
There was a problem hiding this comment.
good question, this is what I considered:
- this keeps the same content (writing only the input text without full country name)
- in terms of accessibility is a much simpler structure and more direct semantics
- less content (no options)
Contributor
There was a problem hiding this comment.
I find this makes the component even more complex (which it already is). The options (and the Autocomplete) are already there and I don't see a valid reason as to why not use it directly. As for the presentation content, I find it strange that the read only content is different from the active state (I know this is already how the component currently is and not your implementation). I also checked figma and the read-only content is the same as the active state. So this makes a good opportunity to align them.
| renderPrefix={countryCode.renderPrefix ?? DefaultPrefix} | ||
| /> | ||
| <div className={classes['country-code-autocomplete-shell']}> | ||
| <input |
Contributor
There was a problem hiding this comment.
What is the purpose of this hidden input ?
Member
Author
There was a problem hiding this comment.
Very good point, thanks.
It's only to keep the onChange sending the same target.
If we change to react state, we could remove the setVersion hack but it would break the onChange contract
I don't have a strong opinion / reason to prefer one over the other, wdyt?
Contributor
There was a problem hiding this comment.
That is a valid point about the API !
We also changed the type of the country code ref to be HTMLInputElement instead of HTMLSelectElement | HTMLInputElement, which makes them breaking changes :/
I think this change need to be part of the next major (planned for early Q3)
Contributor
There was a problem hiding this comment.
I will change the target branch to next and keep you posted !
| disabled={disabled} | ||
| size={size} | ||
| className={clsx( | ||
| classes['country-code'], |
Contributor
There was a problem hiding this comment.
can we merge these two classNames ?
| classes['country-code'], | ||
| classes['country-code-autocomplete'], | ||
| )} | ||
| comboboxClassName={classes['country-code-combobox']} |
Contributor
There was a problem hiding this comment.
instead of passing down a className prop, what do you think about using a [role="combobox] selector to target the input ?
sirineJ
reviewed
May 28, 2026
| flex: 0 0 auto; | ||
| margin-right: -1px; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Can we make the flags rectangular instead of circular ? 🙏
Suggested change
| .country-code-autocomplete-shell [role="option"] img { | |
| height: var(--cui-icon-sizes-mega); | |
| width: var(--cui-icon-sizes-giga); | |
| border-radius: var(--cui-border-radius-bit); | |
| } |
missating
reviewed
May 28, 2026
| countryCode={value as FlagProps['countryCode']} | ||
| alt="" | ||
| className={className} | ||
| width={16} |
Contributor
There was a problem hiding this comment.
The Flag component already defaults to a width of 16, can we keep that here instead of overriding?
| disabled={disabled} | ||
| readOnly={readOnly} | ||
| tabIndex={-1} | ||
| aria-hidden="true" |
Contributor
There was a problem hiding this comment.
Since type="hidden" already handles non-focusability and accessibility tree exclusion, are the last three props needed here?
Details: - Simplified the rendering of the prefix in ComboboxInput tests. - Updated test assertions to check for visibility instead of test ID. - Removed unused `comboboxClassName` prop from ComboboxInput. - Enhanced PhoneNumberInput to render the country code as a read-only input when specified. - Adjusted styles for country code autocomplete to improve layout and responsiveness.
it was introduced because of a conflict with select now removed
simplify styles further
Member
Author
|
Hey many thanks for the great review... I think everything was tackled / answered, lmk wdyt! |
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
3 participants
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.
Purpose
People choosing their country of phone number sometimes have to scroll / swipe a lot to find their country
With this variant, the users can filter their country by typing any part of it (including prefix and screen keyboard in mobile) instead of finding with only the first letter if they have a keyboard
Approach and changes
Demo: https://oss-circuit-ui-git-phone-input-autocomplete.sumup-vercel.app/?path=/story/forms-autocompleteinput--base
Definition of done