feat(ui): add multi-select and bulk actions for packages#1672
feat(ui): add multi-select and bulk actions for packages#1672serhalp merged 61 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
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
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.
app/router.options.ts
Outdated
| // 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?
There was a problem hiding this comment.
I must admit I don't understand why navigation from /foo#bar to /foo#baz still works based on reading this code, but it does seem to work!
I would vote for some sort of a test with a browser. (We can add the test in a follow-up PR, doesn't need to block this!)
| 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.
app/router.options.ts
Outdated
| // 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 must admit I don't understand why navigation from /foo#bar to /foo#baz still works based on reading this code, but it does seem to work!
I would vote for some sort of a test with a browser. (We can add the test in a follow-up PR, doesn't need to block this!)
|
@serhalp I made the changes, and someone just added a |
serhalp
left a comment
serhalp
left a comment
There was a problem hiding this comment.
Thanks for all your work on this! Can't wait to see all the bulk actions we come up with later
|
very cool feature. thx! 🏆 |
4 participants
🔗 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