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 41 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
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! |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/composables/usePackageSelection.ts (1)
10-22: Canonicalise selection values before persisting to query state.
selectedPackagesreads a capped list, but Line 20-Line 21 can still write duplicates or over-limit values into the URL. Keeping read/write logic symmetrical avoids query-state drift.♻️ Suggested normalisation patch
const selectedPackages = computed<string[]>({ get() { const raw = selectedPackagesParam.value if (!raw) return [] - return raw - .split(',') - .map(p => String(p).trim()) - .filter(Boolean) - .slice(0, MAX_PACKAGE_SELECTION) + return [...new Set( + raw + .split(',') + .map(p => String(p).trim()) + .filter(Boolean), + )].slice(0, MAX_PACKAGE_SELECTION) }, set(pkgs: string[]) { - // Ensure all items are strings before joining - const validPkgs = (Array.isArray(pkgs) ? pkgs : []).map(p => String(p).trim()).filter(Boolean) + const validPkgs = [...new Set( + (Array.isArray(pkgs) ? pkgs : []) + .map(p => String(p).trim()) + .filter(Boolean), + )].slice(0, MAX_PACKAGE_SELECTION) selectedPackagesParam.value = validPkgs.length > 0 ? validPkgs.join(',') : '' }, })
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/components/Package/ActionBar.vueapp/components/Package/Card.vueapp/components/Package/ListToolbar.vueapp/components/Package/SelectionView.vueapp/components/Package/Table.vueapp/components/Package/TableRow.vueapp/composables/usePackageSelection.tsapp/pages/search.vueapp/router.options.tsi18n/locales/en.jsoni18n/locales/it-IT.jsoni18n/schema.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.jsonlunaria/files/it-IT.jsonshared/types/npm-registry.tstest/unit/shared/types/index.spec.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- lunaria/files/en-US.json
- i18n/locales/en.json
- app/components/Package/ActionBar.vue
- app/components/Package/SelectionView.vue
- app/components/Package/ListToolbar.vue
- app/components/Package/Table.vue
- i18n/schema.json
after merge conflict, the v-else-if statement got removed by accident
knowler
left a comment
knowler
left a comment
There was a problem hiding this comment.
This is a partial review. Thank you for considering the accessibility of this feature!
| <td class="pe-2"> | ||
| <PackageSelectionCheckbox | ||
| :package-name="result.package.name" | ||
| :disabled="isMaxSelected && !isSelected" | ||
| :checked="isSelected" | ||
| @change="togglePackageSelection" | ||
| /> | ||
| </td> |
There was a problem hiding this comment.
I would consider moving this to the first column. It’s a lot of work for some users to trace from the start to the end of a table row to match selection with a package name.
| <div | ||
| ref="actionBarRef" | ||
| tabindex="-1" | ||
| aria-keyshortcuts="b" | ||
| class="pointer-events-auto bg-bg shadow-2xl shadow-accent/20 border-2 border-accent/60 p-3 min-w-[300px] rounded-xl flex gap-3 items-center justify-between animate-in ring-1 ring-accent/30" | ||
| > |
There was a problem hiding this comment.
Since this container is focusable (even if programmatically) and it currently is not within a landmark (best practice is to put everything in a landmark), I’m thinking that we should make this a region landmark. We can do so with a named <section> element (either using aria-label or aria-labelledby), since when named that is implicitly a region landmark. Since single character keys often conflict with screen reader shortcuts, this provides another means of navigating to the bar.
| > | ||
| <div aria-live="polite" aria-atomic="true" class="sr-only"> | ||
| {{ $t('action_bar.selection', selectedPackages.length) }}. | ||
| {{ $t('action_bar.shortcut', { key: shortcutKey }) }}. |
There was a problem hiding this comment.
I worry that including the search shortcut message each time could get annoying for screen reader users. It is at least at the end. I might need to think more on this.
|
Thanks a lot for the feedback @knowler Open to making changes for better accessibility. In the meantime, I will apply some of your suggestions and see where we land. |
3 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