chore: update play/pause button of trend to be an icon#1635
chore: update play/pause button of trend to be an icon#1635jycouet wants to merge 2 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThe WeeklyDownloadStats.vue component undergoes a layout and skeleton restructuring. The Sparkline rendering is simplified within a relative container with a reworked skeleton structure that condenses the previous multi-part layout into a smaller, more compact design. The animation control is moved to an absolute-positioned inline button instead of a separate UI area. The container height is standardised to 110 pixels, replacing the previous motion-safe variant. The animation toggle is now conditionally rendered and synchronised with hasWeeklyDownloads. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/Package/WeeklyDownloadStats.vue (1)
270-279: Inlinefocus-visibleutility on chart-open button conflicts with project convention.Line 274 applies
focus-visible:outline-accent/70directly on a<ButtonBase>(which renders a<button>). The project convention is to rely on the globalmain.cssrule forbutton:focus-visiblestyling and avoid per-element inline utilities.♻️ Remove inline focus-visible utility
- class="text-fg-subtle hover:text-fg transition-colors duration-200 inline-flex items-center justify-center min-w-6 min-h-6 -m-1 p-1 focus-visible:outline-accent/70 rounded" + class="text-fg-subtle hover:text-fg transition-colors duration-200 inline-flex items-center justify-center min-w-6 min-h-6 -m-1 p-1 rounded"Based on learnings: "In the npmx.dev project, focus-visible styling for buttons and selects is applied globally via main.css… individual buttons or selects in Vue components should not rely on inline focus-visible utility classes like focus-visible:outline-accent/70."
| <ButtonBase | ||
| v-if="hasWeeklyDownloads" | ||
| type="button" | ||
| size="small" | ||
| class="hidden motion-safe:inline-flex absolute top-0 inset-ie-0 !p-1 !border-0 !bg-transparent hover:!bg-transparent text-fg-subtle hover:text-fg transition-colors duration-200 focus-visible:outline-accent/70 rounded" | ||
| :title=" | ||
| hasSparklineAnimation | ||
| ? $t('package.trends.pause_animation') | ||
| : $t('package.trends.play_animation') | ||
| }} | ||
| " | ||
| :classicon="hasSparklineAnimation ? 'i-lucide:pause' : 'i-lucide:play'" | ||
| :aria-pressed="hasSparklineAnimation" | ||
| @click="toggleSparklineAnimation" | ||
| > | ||
| <span class="sr-only"> | ||
| {{ | ||
| hasSparklineAnimation | ||
| ? $t('package.trends.pause_animation') | ||
| : $t('package.trends.play_animation') | ||
| }} | ||
| </span> | ||
| </ButtonBase> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find ButtonBase component
fd -i "buttonbase" --type fRepository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Check for ButtonBase definition in components
find app/components -name "*Button*" -type f | head -20Repository: npmx-dev/npmx.dev
Length of output: 136
🏁 Script executed:
# Search for ButtonBase prop definitions to see if ghost/unstyled variant exists
rg "ButtonBase" app/components --type vue -A 3 | head -80Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
# Check main.css for focus-visible rule
cat app/assets/main.css | grep -A 3 "focus-visible"Repository: npmx-dev/npmx.dev
Length of output: 222
🏁 Script executed:
# Search for ButtonBase in the WeeklyDownloadStats file itself
rg "ButtonBase" app/components/Package/WeeklyDownloadStats.vue -B 5 -A 2Repository: npmx-dev/npmx.dev
Length of output: 1271
🏁 Script executed:
# Search for ButtonBase component definition across entire codebase
rg "ButtonBase" --type-list | head -5Repository: npmx-dev/npmx.dev
Length of output: 450
🏁 Script executed:
# Search for ButtonBase with broader approach
rg "ButtonBase" -g "*.vue" -B 2 -A 5 | head -100Repository: npmx-dev/npmx.dev
Length of output: 7100
🏁 Script executed:
# Check if ButtonBase is imported from elsewhere
rg "export.*ButtonBase" -g "*.vue" -g "*.ts" -g "*.js"Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# List all components in app/components to understand structure
ls -la app/components/*.vue 2>/dev/null | head -20Repository: npmx-dev/npmx.dev
Length of output: 1742
🏁 Script executed:
# Search for ButtonBase.vue in subdirectories
find app/components -name "*[Bb]utton*" -type fRepository: npmx-dev/npmx.dev
Length of output: 136
🏁 Script executed:
# Search for ButtonBase import or definition
rg "ButtonBase" --type vue --type ts --type js -B 1 | grep -E "(import|export|define)" | head -20Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
# Check if ButtonBase is from external package
rg "ButtonBase" package.jsonRepository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Look at the imports in WeeklyDownloadStats.vue
head -50 app/components/Package/WeeklyDownloadStats.vue | rg "import|from"Repository: npmx-dev/npmx.dev
Length of output: 445
🏁 Script executed:
# Search for ButtonBase without type restrictions
rg "ButtonBase" app/components | head -20Repository: npmx-dev/npmx.dev
Length of output: 1398
🏁 Script executed:
# Check nuxt configuration for auto-imports
find . -name "nuxt.config.*" -o -name "components.config.*" | head -5Repository: npmx-dev/npmx.dev
Length of output: 99
🏁 Script executed:
# Look for component definitions (defineComponent, <script setup>)
rg "name.*ButtonBase|component.*ButtonBase" -iRepository: npmx-dev/npmx.dev
Length of output: 388
🏁 Script executed:
# Check app/components directory structure
find app/components -type d | head -30Repository: npmx-dev/npmx.dev
Length of output: 422
🏁 Script executed:
# Let's check the actual WeeklyDownloadStats.vue file to understand ButtonBase
head -200 app/components/Package/WeeklyDownloadStats.vueRepository: npmx-dev/npmx.dev
Length of output: 5249
🏁 Script executed:
# List Button components directory
ls -la app/components/Button/Repository: npmx-dev/npmx.dev
Length of output: 292
🏁 Script executed:
# Check ButtonBase.vue definition
cat app/components/Button/ButtonBase.vueRepository: npmx-dev/npmx.dev
Length of output: 129
🏁 Script executed:
# Check Button/Base.vue
cat app/components/Button/Base.vueRepository: npmx-dev/npmx.dev
Length of output: 2078
🏁 Script executed:
# Verify how ButtonBase is referenced - might be aliased
rg "ButtonBase" --no-filename | head -5Repository: npmx-dev/npmx.dev
Length of output: 306
🏁 Script executed:
# Check nuxt config for component auto-imports or aliases
cat nuxt.config.ts | grep -A 20 "component"Repository: npmx-dev/npmx.dev
Length of output: 1278
🏁 Script executed:
# Check the full main.css for focus-visible rules for buttons
grep -A 10 "button.*focus-visible\|select.*focus-visible" app/assets/main.cssRepository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Verify the exact line in WeeklyDownloadStats.vue we're reviewing
sed -n '315,345p' app/components/Package/WeeklyDownloadStats.vueRepository: npmx-dev/npmx.dev
Length of output: 1317
🏁 Script executed:
# Check if focus-visible rule exists in main.css - try different search
cat app/assets/main.css | grep -i "focus" | head -20Repository: npmx-dev/npmx.dev
Length of output: 94
🏁 Script executed:
# Check ButtonBase usage patterns - are there any instances without !important overrides?
rg "ButtonBase" app/components -A 5 | grep -B 2 "!p-\|!border-\|!bg-" | head -40Repository: npmx-dev/npmx.dev
Length of output: 1052
🏁 Script executed:
# Search for other instances with inline focus-visible utilities
rg "focus-visible:outline" app/components --type vue -B 2 -A 2 | head -50Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
# Check if plain button elements are used elsewhere in the codebase for similar purposes
rg "<button" app/components -A 3 | grep -B 1 "class.*motion-safe\|absolute.*top-0" | head -30Repository: npmx-dev/npmx.dev
Length of output: 478
🏁 Script executed:
# Verify the main.css focus-visible rule applies to all buttons and selects
cat app/assets/main.css | head -100Repository: npmx-dev/npmx.dev
Length of output: 3150
🏁 Script executed:
# Verify the full focus-visible rule context in main.css
grep -B 5 -A 10 "focus-visible" app/assets/main.css | head -30Repository: npmx-dev/npmx.dev
Length of output: 486
🏁 Script executed:
# Check if there are other ButtonBase instances with similar !important patterns
rg "ButtonBase" app/components -A 6 | grep "!\|classicon" | wc -lRepository: npmx-dev/npmx.dev
Length of output: 63
🏁 Script executed:
# Verify the suggested plain button approach with icon - check similar patterns
rg "<button.*classicon|<button.*i-lucide" app/components -A 5 | head -30Repository: npmx-dev/npmx.dev
Length of output: 43
Remove inline focus-visible utility — it conflicts with the global focus-visible rule in main.css.
The inline focus-visible:outline-accent/70 class contradicts project convention. Focus styling for buttons is handled globally via the :focus-visible rule in main.css and should not be duplicated at the component level.
Additionally, the five !important overrides (!p-1 !border-0 !bg-transparent hover:!bg-transparent) are fragile. Since ButtonBase does not expose an unstyled or ghost variant, consider using a plain <button> instead to avoid fighting ButtonBase's default styles.
♻️ Consider using a plain button to avoid !important overrides
- <ButtonBase
+ <button
v-if="hasWeeklyDownloads"
type="button"
- size="small"
- class="hidden motion-safe:inline-flex absolute top-0 inset-ie-0 !p-1 !border-0 !bg-transparent hover:!bg-transparent text-fg-subtle hover:text-fg transition-colors duration-200 focus-visible:outline-accent/70 rounded"
+ class="hidden motion-safe:inline-flex absolute top-0 inset-ie-0 p-1 text-fg-subtle hover:text-fg transition-colors duration-200 rounded"
:title="
hasSparklineAnimation
? $t('package.trends.pause_animation')
: $t('package.trends.play_animation')
"
- :classicon="hasSparklineAnimation ? 'i-lucide:pause' : 'i-lucide:play'"
:aria-pressed="hasSparklineAnimation"
`@click`="toggleSparklineAnimation"
>
+ <span :class="hasSparklineAnimation ? 'i-lucide:pause' : 'i-lucide:play'" class="size-4" aria-hidden="true" />
<span class="sr-only">
{{
hasSparklineAnimation
? $t('package.trends.pause_animation')
: $t('package.trends.play_animation')
}}
</span>
- </ButtonBase>
+ </button>
|
I prefer the effect of the icons. |
2 participants
🔗 Linked issue
Maybe a follow up on #1539
🧭 Context
"Pause animation" & "Play animation" is rather big on this screen, maybe icons with these alt text can work better.
📚 Description
Feel free to discard.