feat(ui): add multi-select and bulk actions for packages#1672
feat(ui): add multi-select and bulk actions for packages#1672MatteoGabriele wants to merge 58 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
knowler
left a comment
There was a problem hiding this comment.
Looks good to me from an accessibility perspective. We can always iterate/tweak things later. Others should review the Vue/JS before we merge.
serhalp
left a comment
There was a problem hiding this comment.
This is an awesome feature! It looks and feels great. Just found a few issues.
| // Don't scroll for other param changes (filters, pagination, etc.) | ||
| if (to.path === from.path) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
I believe this breaks the hash anchor navigation logic just below this, by returning too early before we can check it.
My hunch is always to add a test for the behaviour that almost broke when this occurs, as it's a sign we're missing valuable coverage!
There was a problem hiding this comment.
Navigation with hashes works. I can add tests for the scroll behavior: should I go with e2e or unit tests?
| const selectedPackagesParam = useRouteQuery<string>('selection', '', { mode: 'push' }) | ||
| const showSelectionViewParam = useRouteQuery<string>('view', '', { mode: 'push' }) | ||
|
|
||
| // Parse URL param into array of package names |
There was a problem hiding this comment.
🤔 I'm all for the URL-as-source-of-truth-for-state school of thought, but I wonder if selection within search results falls into the ephemeral state bucket that should not be in the URL and consequently in the browser navigation history?
When I play around with it, it admittedly looks cool to hit back and forward across those selections/deselections, but it feels surprising.
I'd be curious to hear other opinions, but my hunch would be not to include it in the URL.
There was a problem hiding this comment.
I need to include it in the URL; otherwise, I won't be able to refresh and get the list from your selection. This is especially helpful if you refresh, navigate to other pages, and then come back. It also makes the selection page shareable. I agree that perhaps it shouldn't be pushed into history, but it can be replaced. Let me know what you think.
I have already made this change.
There was a problem hiding this comment.
Ah yeah, the selection page feels different to me than the existing search results page with selections.
There was a problem hiding this comment.
Is that a good thing or a bad thing?
There was a problem hiding this comment.
Sorry, that wasn't very clear. I meant that on the new selection view I agree selections should be part of the URL and navigation history, but on the normal full search results view selection state feels more ephemeral. 🤔
There was a problem hiding this comment.
I tried using useState in a previous version, but the saved state felt wrong, and the data would appear when you didn't expect it to. I believe this one is best suited to a URL: I don't think this functionality fits any other storage options that don't require complex babysitting. URL, in a way, is straightforward, managed by navigation, and predictable. You go back and find what you left; you arrive there from another route and see a clean slate. That said, if you have another idea, I'm happy to try it out and see how it feels.
| }, | ||
| }) | ||
|
|
||
| const isMaxSelected = computed(() => selectedPackages.value.length >= MAX_PACKAGE_SELECTION) |
There was a problem hiding this comment.
nit: this is super nitpicky but (especially since it's exported) this could be more expressive of the intent rather than the implementation, something like:
| const isMaxSelected = computed(() => selectedPackages.value.length >= MAX_PACKAGE_SELECTION) | |
| const shouldDisableSelection = computed(() => selectedPackages.value.length >= MAX_PACKAGE_SELECTION) |
There was a problem hiding this comment.
I can change it, but I disagree with this one. This is a composable property that tells the component that the maximum selectable items have been reached (perhaps the name could be tweaked), but it is not the job of the composable to tell you that it is disabled, since other parts of the UI might be bound to a completely different functionality or state. Ex: one component disables a checkbox, another component simply displays a tooltip.
There was a problem hiding this comment.
I can see that. Maybe canSelectMore?
| function isPackageSelected(packageName: string): boolean { | ||
| return selectedPackages.value.includes(String(packageName).trim()) |
There was a problem hiding this comment.
nit: the type says packageName is a string but we're casting it to a string. Is the type wrong or the cast unnecessary?
There was a problem hiding this comment.
Good catch! Those are leftovers from when I was trying to hack some stuff around 👍
| function togglePackageSelection(packageName: string) { | ||
| const safeName = String(packageName).trim() |
| selectedPackages.value.map(name => | ||
| $fetch(`/api/registry/package-meta/${encodeURIComponent(name)}`) | ||
| .then(response => ({ package: response })) | ||
| .catch(() => []), |
There was a problem hiding this comment.
This fallback's type is incorrect.
| .catch(() => []), | |
| .catch(() => ({})), |
This may be another good case for test coverage!
btw we can type this a little closer to the fetch response by passing a generic: https://npmx.dev/package/ofetch#user-content-type-friendly
app/components/Package/ActionBar.vue
Outdated
| <button | ||
| @click="clearSelectedPackages" | ||
| class="flex items-center ms-1 text-fg-muted hover:(text-fg bg-accent/10) p-1.5 rounded-lg transition-colors" | ||
| aria-label="Close action bar" |
There was a problem hiding this comment.
🔗 Linked issue
resolves #1509
🧭 Context
Added a multi-select feature to the search page that allows users to select multiple packages and perform bulk actions on them. Currently supports comparing selected packages, with the possibility of adding more actions in the future.
📚 Description
At the moment, the logic is locked at a maximum of 4 selectable items. This won't scale, but for now, to reduce complexity, it will mimic what's needed by the Compare page, which is the only current functionality available in the multi-select.
My take is to re-think this along the way, when and if another action gets added.
Screen.Recording.2026-02-27.at.18.33.58.mov