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

core: refacto combo box #779

Merged
merged 1 commit into from
Jan 8, 2025
Merged

core: refacto combo box #779

merged 1 commit into from
Jan 8, 2025

Conversation

clarani
Copy link
Contributor

@clarani clarani commented Dec 23, 2024

closes #778

Here are the main changes:

  • the combo box takes now a value of type T and not a string anymore
  • the combo box has a default behavior (filters on the labels) but a custom behaviour can be used
  • the hook useOutsideClick replaces the previous handleParentDivBlur function
  • a new story has been added to test the custom behavior

@clarani clarani requested a review from a team as a code owner December 23, 2024 10:15
@clarani clarani requested a review from emersion December 23, 2024 10:15
@clarani clarani force-pushed the cni/778-refacto-combo-box branch from 0c50296 to 3ebcc5e Compare December 23, 2024 10:20
@clarani clarani force-pushed the cni/778-refacto-combo-box branch 2 times, most recently from 063a491 to c752918 Compare January 6, 2025 14:55
@clarani clarani requested a review from emersion January 6, 2025 15:36
@clarani clarani force-pushed the cni/778-refacto-combo-box branch 4 times, most recently from b88b73a to b29bc8b Compare January 7, 2025 08:35
emersion
emersion reviewed Jan 7, 2025
View reviewed changes
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

This is so much better than the old code, thanks a bunch!

Here are a few comments.

@emersion emersion mentioned this pull request Jan 7, 2025
Closed
emersion
emersion approved these changes Jan 7, 2025
View reviewed changes
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

LGTM :)

@clarani
core: refacto combo box
60bea9c 
Here are the main changes:
- the combo box takes now a value of type T and not a string anymore
- the combo box has a default behavior (filters on the labels) but a custom behaviour can be used
- the hook useOutsideClick replaces the previous handleParentDivBlur function
- a new story has been added to test the custom behavior

Signed-off-by: Clara Ni <[email protected]>
@clarani clarani force-pushed the cni/778-refacto-combo-box branch from d9834ae to 60bea9c Compare January 8, 2025 08:30
SharglutDev
SharglutDev approved these changes Jan 8, 2025
View reviewed changes
Copy link
Contributor

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

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

Lgtm and tested, great job !

@clarani clarani added this pull request to the merge queue Jan 8, 2025
Merged via the queue into dev with commit 3e679f7 Jan 8, 2025
7 checks passed
@clarani clarani deleted the cni/778-refacto-combo-box branch January 8, 2025 08:53
This was referenced Jan 14, 2025
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emersion emersion emersion approved these changes

@SharglutDev SharglutDev SharglutDev approved these changes

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

Successfully merging this pull request may close these issues.

Refacto combo box
3 participants
@clarani @emersion @SharglutDev