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.

Sign up for GitHub

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

Jump to bottom

Integrating MUI Autocomplete for searching and filtering countries #54

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Integrating MUI Autocomplete for searching and filtering countries #54

wants to merge 21 commits into from

Conversation

ja153903
Copy link

@ja153903 ja153903 commented Feb 24, 2023 •
edited
Loading

This PR integrates an alternative modal to the FlagsMenu that allows for the selection of a calling code via countries. This method integrates MUI's Autocomplete component to allow users to type and filter the list of countries to easily find their desired calling code.

Comments are appreciated!

@ja153903 ja153903 changed the title Integrating MUI Autocomplete for filtering countries Integrating MUI Autocomplete for searching and filtering countries Feb 24, 2023
@ja153903 ja153903 mentioned this pull request Feb 26, 2023
Open
@viclafouch
Copy link
Owner

image

viclafouch
viclafouch reviewed Mar 14, 2023
View reviewed changes
src/index.tsx Outdated
@@ -214,6 +216,20 @@ const MuiTelInput = React.forwardRef(
{...MenuProps}
/>
) : null}
{!disableDropdown && allowSearch ? (
<FlagsAutocomplete
Copy link
Owner

Choose a reason for hiding this comment

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

too much duplications.. Would be nice to keep everything less verbose, and having only a logic for the visibility of the autocomplete (visible or not visible).

viclafouch
viclafouch reviewed Mar 14, 2023
View reviewed changes
src/components/FlagsAutocomplete/FlagsAutocomplete.tsx Outdated
<ClickAwayListener onClickAway={onClose}>
<Autocomplete
autoHighlight
filterOptions={(options, { inputValue }) => {
Copy link
Owner

Choose a reason for hiding this comment

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

move this out of the JSX plz

@ja153903
Copy link
Author

@viclafouch Thanks for looking through! I'll make the updates.

emkayy
emkayy reviewed Mar 16, 2023
View reviewed changes
src/components/FlagsAutocomplete/FlagsAutocomplete.styled.ts Outdated
Copy link

Choose a reason for hiding this comment

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

Instead of having all styles "hardcoded", please make them optional / customisable and provide the possibility to add custom CSS classes to each element.

Copy link
Author

@ja153903 ja153903 Mar 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@emkayy I think there needs to be baseline styling so that it matches the current FlagMenu. Do you have components of the autocomplete in mind that you'd think would benefit from making it optional?

Copy link

Choose a reason for hiding this comment

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

Baseline styling yes, but I think it's important to be able to customize it. Unfortunately I don't know what you mean by "Do you have components of the autocomplete in mind ...".

Copy link
Author

Choose a reason for hiding this comment

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

If you read the source code for FlagsAutocomplete there are different components in there that would require some styling. I'm asking which ones you would actually consider important to have customized styling for.

It might be a good idea for you to clone my fork and see what might be important for you to have some control over.

Copy link

Choose a reason for hiding this comment

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

I think for the input it would be better to have a plain input without a label that spans across the whole popper width.
However, for complete customization, the renderInput prop for Autocomplete would be great, like
<MuiTelInput flagPopupProps={{ AutocompleteProps: {renderInput: ()=> MyCustomInput }}} />

For the popper I would use the theme default style and not do any custom styling. But if you do, you should also provide props to override them.

ja153903 added 3 commits March 16, 2023 19:19
@ja153903
fix: addressing the following code comments:
0e37967 
1. Fixing the issue with getting the appropriate option label
2. Moving logic out of jsx for readability
@ja153903
fix: moving getOptionLabel outside of jsx
2f6dc34
@ja153903
fix: addressing too much duplicated code comment
f1d1805
@ja153903 ja153903 requested review from viclafouch and emkayy and removed request for viclafouch March 17, 2023 15:27
ja153903
ja153903 commented Mar 17, 2023
View reviewed changes
src/index.tsx Show resolved Hide resolved
@ja153903 ja153903 requested review from viclafouch and removed request for emkayy March 17, 2023 15:29
@riadhtrvlcode
Copy link

i need this feature 🥲

@ja153903
Copy link
Author

@viclafouch I've addressed your initial concerns. Let me know if there's anything else you want me to consider looking into.

@viclafouch
Copy link
Owner

Let me take a look this weekend ;)

ty !!

@emkayy
Copy link

emkayy commented Apr 5, 2023

At the moment I have to issues with the current implementation (apart from styling ;) )

  • For me (Chrome, Win10) the autocomplete doesn't work because MUI Menu captures keyboard inputs to jump to the item that starts with the letter of the key pressed. So my key strokes never even reach the input.
  • If you have a default or pre-selected country, the list automatically scrolls down and you can't even see the input on top.
    muitelinput

@ja153903
Copy link
Author

ja153903 commented Apr 5, 2023

At the moment I have to issues with the current implementation (apart from styling ;) )

  • For me (Chrome, Win10) the autocomplete doesn't work because MUI Menu captures keyboard inputs to jump to the item that starts with the letter of the key pressed. So my key strokes never even reach the input.

  • If you have a default or pre-selected country, the list automatically scrolls down and you can't even see the input on top.
    muitelinput

        [
      
    
        ![muitelinput](https://user-images.githubusercontent.com/731228/230065387-3fd3ad5c-4fe3-4c6c-a40e-347879d69b4f.gif)
      ](https://user-images.githubusercontent.com/731228/230065387-3fd3ad5c-4fe3-4c6c-a40e-347879d69b4f.gif)
    
    
      
        
          
        
        
          
          
        
      
      [
        
          
        
      ](https://user-images.githubusercontent.com/731228/230065387-3fd3ad5c-4fe3-4c6c-a40e-347879d69b4f.gif)

Is this your own implementation? I'm quite sure this isn't how I've implemented this.

@ManuC84
Copy link

ManuC84 commented Aug 10, 2023

@ja153903 Hey! I just noticed we're using this library on one of our projects and I'm being asked to add this feature. Do you think this will be implemented any time soon or should I look for something else/build something from scratch?

@mi-mazouz
Copy link

Hello ! We need this feature 🙏

@ericfeldman-RCM
Copy link

Would love to use this library, just need that search ability added!

@viclafouch
Copy link
Owner

Hey ! Please resolve conflict and then I will merge the PR !

Sorry for the delay !

Ty !

viclafouch
viclafouch reviewed Oct 12, 2023
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
viclafouch
viclafouch reviewed Oct 12, 2023
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
viclafouch
viclafouch reviewed Oct 12, 2023
View reviewed changes
src/index.tsx Outdated Show resolved Hide resolved
viclafouch
viclafouch reviewed Oct 12, 2023
View reviewed changes
vite.config.ts Outdated Show resolved Hide resolved
@ja153903 ja153903 requested a review from viclafouch October 14, 2023 16:13
@riadhtrvlcode
Copy link

we need this feature please when this feature is available ?

@jakeisnt
Copy link

Would love to get this in as well! Let me know if there is a way to help.

@graysonhicks
Copy link

Looks like just a few conflicts from getting this merged. Would be a great addition to the library. Following for release! 😄

@yotamberk
Copy link

+1

2 similar comments
@maciejtatol
Copy link

+1

@EvgenyPolyakov
Copy link

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emkayy emkayy emkayy left review comments

@viclafouch viclafouch Awaiting requested review from viclafouch

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.