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.

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

fix: settings form select menu #9179

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

harshrajeevsingh
Copy link
Contributor

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

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Enhanced select components across settings forms with improved search and selection visuals to provide better user experience.

  • Added search functionality to currency and address form dropdowns with fixed 220px width to prevent UI shrinking
  • Implemented country flags in address form dropdown for better visual identification
  • Added needIconCheck prop to MenuItemSelect to optionally hide check icons while maintaining selected state highlighting
  • Replaced MenuItem with MenuItemSelect across forms for consistent selection feedback
  • Disabled check icons for boolean fields since they already have true/false icons

6 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

}));
countries.unshift({
label: 'No country',
value: '',
Icon: IconCircleOff,
});
const defaultDefaultValue = {
addressStreet1: "''",
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: addressStreet1 default value of "''" contains nested quotes which may cause issues. Consider using just '' or an empty string

@guillim guillim self-requested a review December 23, 2024 09:35
@guillim
Copy link
Contributor

guillim commented Dec 23, 2024

Thanks for your PR. I will look into it in the coming days, but from my first glance, it looks pretty good !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dropdown Tick box Currency Search
2 participants