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

Currency Search #8647

Closed
guillim opened this issue Nov 21, 2024 · 13 comments · Fixed by #9179
Closed

Currency Search #8647

guillim opened this issue Nov 21, 2024 · 13 comments · Fixed by #9179
Assignees
@guillim
Labels
-PR: wip prio: low type: design improvement

Comments

@guillim
Copy link
Contributor

guillim commented Nov 21, 2024 •
edited
Loading

We would like to have the Search feature in Currency field metadata

Current behavior

You need to scroll down, and it can be very long to select currencies at the bottom of the dropdown

Example:

Image

Image

Expected behavior

Search bar like on the Country selection dropdown

Technical inputs

We should try to see if the option exist on the current component, and then add it. Otherwise, we should have a look at the other existing components to find one that would match the requirements. Last case scneario, let's create a dedicated component

As view with @Bonapara
@Bellahuang3715
Copy link

Hi, I would like to take on this issue, can you please assign it to me? Thank you!

@FelixMalfait
Copy link
Member

Sure @Bellahuang3715 thanks!

@BOHEUS
Copy link
Contributor

BOHEUS commented Nov 21, 2024

Related to #7038 (it should be implemented then according to tasks list but for some reason it wasn't)

@guillim
Copy link
Contributor Author

guillim commented Nov 25, 2024

Did you start working on it @Bellahuang3715 ?

@Bellahuang3715
Copy link

@guillim Yes! Aiming to wrap it up by tomorrow!

@guillim guillim mentioned this issue Nov 25, 2024
Closed
2 tasks
@guillim guillim self-assigned this Nov 25, 2024
@guillim guillim mentioned this issue Nov 26, 2024
Closed
@charlesBochet charlesBochet moved this from 🆕 New to 🔖 Planned in Product development ✅ Nov 26, 2024
@harshrajeevsingh
Copy link
Contributor

Hi @guillim I see there is no update yet. So, I would like to work on this.

Image

  1. The Select component already accepts withSearchInput. So we can directly use that.
  2. For Dropdown Tick box #8649, I think it would be better to use MenuItemSelect instead of MenuItem inside the Select component. It would be useful across all the select dropdowns. For example:

Image
Image

@guillim
Copy link
Contributor Author

guillim commented Dec 9, 2024

@harshrajeevsingh I agree, let's try to reuse the components from twenty-ui in order to achieve this issue. You can start working on this currency search issue if you want. I will be active on it as well, so don't hesitate to ping me and we can collaborate here to decide which component is best to re-use !

@harshrajeevsingh
Copy link
Contributor

harshrajeevsingh commented Dec 10, 2024 •
edited
Loading

@guillim Sure! I will take a look at this tomorrow and will let you know which could be the best solution to tackle both the issues.

@harshrajeevsingh
Copy link
Contributor

@guillim I looked into this again and I still think that using MenuItemSelect instead of MenuItem in the Select component is a better choice. Here is why:

  1. First of all we get an IconCheck.
  2. It uses StyledMenuItemBase, so the text will get truncated on smaller/fixed width's dropdown and on hover we get the desired tooltip giving the full context. So, now we can remove the dropdown width's 'auto' and use a fixed width without making the dropdown look broader.

Here I made changes for the #8751
Lemme know if it looks good!

Image

@guillim
Copy link
Contributor Author

guillim commented Dec 12, 2024

where are the changes #8751 points to an already closed pr

@harshrajeevsingh
Copy link
Contributor

@guillim I don't see it anymore. I'm on the main branch. Can you recheck from your side?

Image

@guillim
Copy link
Contributor Author

guillim commented Dec 12, 2024

I see this on the main branch, which is the current behaviour described at the top of this issue. So everything's normal. We still need to do the feature :)

Image

@harshrajeevsingh harshrajeevsingh mentioned this issue Dec 21, 2024
Merged
@harshrajeevsingh
Copy link
Contributor

@guillim I made a PR here #9179
Sorry for the delay! I was busy with personal work. But I will be active from now onwards :)

guillim pushed a commit that referenced this issue Dec 24, 2024
@harshrajeevsingh
fix: settings form select menu (#9179)
fe6948b 
Closes: #8647 
Closes: #8649 

**Changes & Why**

1. Added a Search Input to `SettingsDataModelFieldAddressForm` &
`SettingsDataModelFieldCurrencyForm` as `Select` component already
accepts it as a prop.
2. Gave a fixed width to the dropdown of both the above components to
ensure it doesn't shrink on search for the menu items with low word
count.
3. Added countries Flag to `SettingsDataModelFieldAddressForm`. 
4. Replaced `MenuItem` with `MenuItemSelect` to get the desired
highlighted background for the selected item with `IconCheck` to
differentiate the current selected item. This is useful across all the
select components throughout the app.
5. I realized that in some components we might not need IconCheck and
only need a highlighted background for the selected item. For ex:
`SettingsDataModelFieldBooleanForm` . Therefore, I created a prop
`needIconCheck` with default as true so it doesn't break the existing
`MenuItemSelect` and we can pass that prop as false wherever needed.

[Screencast from 2024-12-21
12-08-08.webm](https://github.com/user-attachments/assets/4f8070a8-f339-4556-a137-bbbad58b171c)
@github-project-automation github-project-automation bot moved this from 🔖 Planned to ✅ Done in Product development ✅ Dec 24, 2024
@guillim guillim moved this from ✅ Done to 🔦 In QA in Product development ✅ Dec 24, 2024
lucasbordeau pushed a commit that referenced this issue Dec 24, 2024
@harshrajeevsingh @lucasbordeau
fix: settings form select menu (#9179)
26649f7 
Closes: #8647 
Closes: #8649 

**Changes & Why**

1. Added a Search Input to `SettingsDataModelFieldAddressForm` &
`SettingsDataModelFieldCurrencyForm` as `Select` component already
accepts it as a prop.
2. Gave a fixed width to the dropdown of both the above components to
ensure it doesn't shrink on search for the menu items with low word
count.
3. Added countries Flag to `SettingsDataModelFieldAddressForm`. 
4. Replaced `MenuItem` with `MenuItemSelect` to get the desired
highlighted background for the selected item with `IconCheck` to
differentiate the current selected item. This is useful across all the
select components throughout the app.
5. I realized that in some components we might not need IconCheck and
only need a highlighted background for the selected item. For ex:
`SettingsDataModelFieldBooleanForm` . Therefore, I created a prop
`needIconCheck` with default as true so it doesn't break the existing
`MenuItemSelect` and we can pass that prop as false wherever needed.

[Screencast from 2024-12-21
12-08-08.webm](https://github.com/user-attachments/assets/4f8070a8-f339-4556-a137-bbbad58b171c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@guillim guillim

Labels
-PR: wip prio: low type: design improvement
Projects
Product development ✅
Status: 🔦 In QA
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: settings form select menu harshrajeevsingh/twenty
5 participants
@FelixMalfait @guillim @harshrajeevsingh @BOHEUS @Bellahuang3715