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: Multiselect interactivity and form bug fixes #982

Merged
merged 3 commits into from
Aug 22, 2023

Conversation

bentilling
Copy link
Contributor

@bentilling bentilling commented Aug 13, 2023

πŸ“‘ Description

UI fixes

  • Added background colour on light mode to avoid see though drop down
  • Only show clear all icon when at least one element selected
  • Fixed bug where clicking in the gap between the items in the dropdown closes the menu
  • Fixed bug where the dropdown is cropped by other divs and modals
  • Reversed chevron direction when dropdown open
  • Added additional contrast when text hovered/selected

Form fixes

  • Clicking the chevron no longer submits a form
  • Added a hidden html multi-select with bound values for form actions

Status

  • Not Completed
  • Completed

βœ… Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • I have checked the page with https://validator.unl.edu/
  • All the tests have passed
  • My pull request is based on the latest commit (not the npm version).

β„Ή Additional Information

@stackblitz
Copy link

stackblitz bot commented Aug 13, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@vercel
Copy link

vercel bot commented Aug 13, 2023

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated (UTC)
flowbite-svelte βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Aug 22, 2023 7:30pm
flowbite-svelte-update βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Aug 22, 2023 7:30pm

@bentilling
Copy link
Contributor Author

I didn't want to make any breaking changes in this PR but if wondering what the maintainers think of the additional suggestions as a follow up

  1. Changing the
    value: (string | number)[] prop to selected: SelectOptionType[]

As the items prop is already SelectOptionType[] this keeps the prop types consistent and simplifies the component which is currently keeping two vars representing the same info in sync.

  1. Removing (or make optional) custom events from inside the component.

Imo we do not want to force action events for those that simply want to bind to the value or use as a form. I suggest we either remove and let the parent component fire an event off the bound selected or alternatively we could add a prop like eventType: (string | null) which we use to optionally setup with a custom type (as at the moment they are all firing the same β€œselected” event which could cause issues if multiple multi-selects exist).

Let me know your thoughts or if better to move this discussion elsewhere!

@hamboomger
Copy link

hamboomger commented Aug 20, 2023

Clicking the chevron no longer submits a form

Without this I can't use Multiselect on production, thank you for fixing that! Also it's transparent for some reason. Hope this PR will get merged soon...

@bmccorm2
Copy link

Thank you for fixing this this!! Looking forward to the merge.
Added a hidden html multi-select with bound values for form actions

@bentilling
Copy link
Contributor Author

Thanks both, @hamboomger yes this also fixes the transparent issue.

@shinokada is there anything you want me to address in this before it is ready to merge?

@shinokada
Copy link
Collaborator

  • Line 26: Can you change the quote to single, not back-tick.

  • LIne 84 change twJoin to twMerge so that $$props.class can overwrite multiSelectClass.

@shinokada
Copy link
Collaborator

Thank you for your contribution.

shinokada pushed a commit that referenced this pull request Aug 23, 2023
* bug fixes and add hidden select element

* pr comments
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.

None yet

4 participants