fix: support sorting algolia search results#1227
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! |
📝 WalkthroughWalkthroughThis pull request implements provider-aware sorting for package search. It adds a new Possibly related PRs
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/components/Package/ListToolbar.vue (1)
186-208: Use UnoCSS preset-icons colon syntax for consistency with the codebase.Lines 186 and 207–208 use dash syntax (
i-carbon-chevron-down,i-carbon-sort-ascending,i-carbon-sort-descending), but colon syntax (i-carbon:chevron-down, etc.) is the standard throughout the codebase and provides better performance optimisation.♻️ Suggested change
- <span class="i-carbon-chevron-down w-4 h-4" /> + <span class="i-carbon:chevron-down w-4 h-4" /> ... - ? 'i-carbon-sort-ascending' - : 'i-carbon-sort-descending' + ? 'i-carbon:sort-ascending' + : 'i-carbon:sort-descending'app/pages/search.vue (1)
114-126: Consider deriving this list from the shared type definition.
ALL_SORT_KEYSduplicates the non-relevance sort keys that are likely already defined in#shared/types/preferences. If theSortKeytype is extended in the future, this list could drift out of sync.
| <!-- Sort direction toggle --> | ||
| <button | ||
| v-if="!searchContext" | ||
| v-if="!searchContext || currentSort.key !== 'relevance'" | ||
| type="button" | ||
| class="p-1.5 rounded border border-border bg-bg-subtle text-fg-muted hover:text-fg hover:border-border-hover transition-colors duration-200 focus-visible:ring-2 focus-visible:ring-fg focus-visible:ring-offset-2 focus-visible:ring-offset-bg" | ||
| :aria-label="$t('filters.sort.toggle_direction')" |
There was a problem hiding this comment.
Remove per-button focus-visible utilities; rely on the global focus styling.
Line 194 adds focus-visible utility classes to a button, which conflicts with the project’s global focus-visible rule for buttons and selects.
✅ Suggested change
- class="p-1.5 rounded border border-border bg-bg-subtle text-fg-muted hover:text-fg hover:border-border-hover transition-colors duration-200 focus-visible:ring-2 focus-visible:ring-fg focus-visible:ring-offset-2 focus-visible:ring-offset-bg"
+ class="p-1.5 rounded border border-border bg-bg-subtle text-fg-muted hover:text-fg hover:border-border-hover transition-colors duration-200"Based on learnings: In the npmx.dev project, ensure that focus-visible styling for button and select elements is implemented globally in app/assets/main.css with the rule: button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Do not apply per-element inline focus-visible utility classes like focus-visible:outline-accent/70 on these elements.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <!-- Sort direction toggle --> | |
| <button | |
| v-if="!searchContext" | |
| v-if="!searchContext || currentSort.key !== 'relevance'" | |
| type="button" | |
| class="p-1.5 rounded border border-border bg-bg-subtle text-fg-muted hover:text-fg hover:border-border-hover transition-colors duration-200 focus-visible:ring-2 focus-visible:ring-fg focus-visible:ring-offset-2 focus-visible:ring-offset-bg" | |
| :aria-label="$t('filters.sort.toggle_direction')" | |
| <!-- Sort direction toggle --> | |
| <button | |
| v-if="!searchContext || currentSort.key !== 'relevance'" | |
| type="button" | |
| class="p-1.5 rounded border border-border bg-bg-subtle text-fg-muted hover:text-fg hover:border-border-hover transition-colors duration-200" | |
| :aria-label="$t('filters.sort.toggle_direction')" |
No description provided.