Skip to content

Feature/svelte portal 301 #302

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
Closed
wants to merge 3 commits into from
Closed

Feature/svelte portal 301 #302

wants to merge 3 commits into from

Conversation

MarcDAFrame
Copy link

Closes #301 .

Summary of major changes

  1. added a new prop fixed
  2. uses popper.js to render it as fixed
  3. doesn't use popper.js if fixed is not set so as to retain the current positioning system

Notes

  • I had to reduce versions of svelte-zoo and install some missing deps to get this to work
  • I didn't pass tests : (
  ✘  3 tests/MultiSelect.test.ts:104:3 › remove all button › has custom title (30.0s)
  ✘  4 tests/MultiSelect.test.ts:84:3 › remove all button › only appears if more than 1 option is selected and removes all selected (30.0s)
  ✘  27 tests/MultiSelect.test.ts:253:3 › multiselect › can select and remove many options (30.0s)
  • npm run package && npm run dev had to run these commands after every change, not sure why but it got the job done

MarcDAFrame and others added 3 commits March 7, 2025 14:17
@MarcDAFrame
added fixed position prop, enabling dropdown to expand beyond the par…
a66d90c 
…ent element
@MarcDAFrame
added test-results to .gitignore, updated readme to include documenta…
7a7442a 
…tion for the new prop, updated how we apply popper.js so that it uses the default rendering system rather than popper.js by default
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
b9e0152 
for more information, see https://pre-commit.ci
@MarcDAFrame
Copy link
Author

Hey @janosh just following up on this : )

hope you like it

janosh
janosh requested changes Mar 14, 2025
View reviewed changes
Copy link
Owner

@janosh janosh left a comment

Choose a reason for hiding this comment

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

thanks for this PR! 👍
two comments below and we definitely need a unit test for this new prop

package.json
@@ -48,14 +48,17 @@
"rehype-slug": "^6.0.0",
"svelte-check": "^4.0.5",
"svelte-multiselect": "^10.3.0",
"svelte-popperjs": "^1.3.2",
Copy link
Owner

Choose a reason for hiding this comment

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

https://github.com/bryanmylee/svelte-popperjs looks to be unmaintained (last commit 2 years ago) and i think i read that the next major Svelte release is expected to break backward compatibility with Svelte 4. so adding this dependency could become an anchor to Svelte 5 in the future. let's try to minimize the number of new packages added for this feature

readme.md
@@ -423,6 +423,11 @@ Full list of props/bindable variables for this component. The `Option` type you

If `maxSelect={1}`, `value` will be the single item in `selected` (or `null` if `selected` is empty). If `maxSelect != 1`, `maxSelect` and `selected` are equal. Warning: Setting `value` does not rendered state on initial mount, meaning `bind:value` will update local variable `value` whenever internal component state changes but passing a `value` when component first mounts won't be reflected in UI. This is because the source of truth for rendering is `bind:selected`. `selected` is reactive to `value` internally but only on reassignment from initial value. Suggestions for better solutions than [#249](https://github.com/janosh/svelte-multiselect/issues/249) welcome!

1. ```ts
fixed: boolean | null = false
Copy link
Owner

Choose a reason for hiding this comment

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

what's the purpose of allowing null here?

also, can we find a more descriptive name than fixed. let's call it overflowParent: hidden | visible = visible or something like that

@janosh janosh mentioned this pull request May 17, 2025
Merged
@janosh
Copy link
Owner

janosh commented May 17, 2025

closing in favor of #306. thanks again for the feature proposal and this first implementation! 👍

@janosh janosh closed this May 17, 2025
@janosh janosh added the duplicate This issue or pull request already exists label May 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janosh janosh janosh requested changes

Assignees
No one assigned
Labels
duplicate This issue or pull request already exists
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Portal / render the multiselect outside of the current DOM structure
2 participants
@MarcDAFrame @janosh