Skip to content

chore: update play/pause button of trend to be an icon #1635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jycouet wants to merge 2 commits into
base: main
Choose a base branch
from
+49 −34
Open

chore: update play/pause button of trend to be an icon #1635

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b06802
step 0
jycouet Feb 24, 2026
c8e0a46
step 7
jycouet Feb 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 49 additions & 34 deletions app/components/Package/WeeklyDownloadStats.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ const config = computed<VueUiSparklineConfig>(() => {
</script>

<template>
<div class="space-y-8 h-[110px] motion-safe:h-[140px]">
<div class="space-y-8 h-[110px]">
<CollapsibleSection id="downloads" :title="$t('package.downloads.title')">
<template #actions>
<ButtonBase
Expand All @@ -282,46 +282,61 @@ const config = computed<VueUiSparklineConfig>(() => {

<div class="w-full overflow-hidden">
<template v-if="isLoadingWeeklyDownloads || hasWeeklyDownloads">
<ClientOnly>
<VueUiSparkline class="w-full max-w-xs" :dataset :config>
<template #skeleton>
<!-- This empty div overrides the default built-in scanning animation on load -->
<div />
</template>
</VueUiSparkline>
<template #fallback>
<!-- Skeleton matching VueUiSparkline layout (title 24px + SVG aspect 500:80) -->
<div class="max-w-xs">
<!-- Title row: fontSize * 2 = 24px -->
<div class="h-6 flex items-center ps-3">
<SkeletonInline class="h-3 w-36" />
</div>
<!-- Chart area: matches SVG viewBox 500:80 -->
<div class="aspect-[500/80] flex items-center">
<!-- Data label (covers ~42% width, matching dataLabel.offsetX) -->
<div class="w-[42%] flex items-center ps-0.5">
<SkeletonInline class="h-7 w-24" />
<div class="relative">
<ClientOnly>
<VueUiSparkline class="w-full max-w-xs" :dataset :config>
<template #skeleton>
<!-- This empty div overrides the default built-in scanning animation on load -->
<div />
</template>
</VueUiSparkline>
<template #fallback>
<!-- Skeleton matching VueUiSparkline layout (title 24px + SVG aspect 500:80) -->
<div class="max-w-xs">
<!-- Title row: fontSize * 2 = 24px -->
<div class="h-6 flex items-center">
<SkeletonInline class="h-3 w-48" />
</div>
<!-- Sparkline line placeholder -->
<div class="flex-1 flex items-end pe-3">
<SkeletonInline class="h-px w-full" />
<!-- Chart area: matches SVG viewBox 500:80 -->
<div class="aspect-[500/80] flex items-center">
<!-- Data label (covers ~42% width, matching dataLabel.offsetX) -->
<div class="w-[42%] flex items-center ps-0.5">
<SkeletonInline class="h-7 w-24" />
</div>
<!-- Sparkline line placeholder -->
<div class="flex-1 flex items-end pe-3">
<SkeletonInline class="h-px w-full" />
</div>
</div>
</div>
<!-- Animation toggle placeholder -->
<div class="w-full hidden motion-safe:flex flex-1 items-end justify-end">
<SkeletonInline class="h-[20px] w-30" />
<!-- Animation toggle button placeholder (matches absolute top-right button) -->
<div class="hidden motion-safe:block absolute top-0 inset-ie-0 p-1">
<SkeletonInline class="size-4 rounded" />
</div>
</div>
</template>
</ClientOnly>

<div v-if="hasWeeklyDownloads" class="hidden motion-safe:flex justify-end p-1">
<ButtonBase size="small" @click="toggleSparklineAnimation">
{{
</template>
</ClientOnly>

<ButtonBase
v-if="hasWeeklyDownloads"
type="button"
Copy link
Member

@knowler knowler Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant, we already add type=button in the component.

Suggested change
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')
Comment on lines +324 to 327
Copy link
Member

@knowler knowler Feb 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Optional for this PR… eventually we’ll fix globally)

A tooltip whose contents are the label for this button (i.e. associated with aria-labelledby) would be better. This is a pattern I’d like to see us change across the board (eventually I’ll file an issue about removing title pretty much everywhere). I’ll note that I think our tooltip implementation is also flawed (i.e. I’m not sure if we add role=tooltip and it doesn’t meet the dismissable requirement of 1.4.13 Content on Hover or Focus Level AA).

Don’t worry about solving that here. If it’s easier to just leave title than use our existing flawed Tooltip solution, we can always circle back to fix it when we address the more pervasive problem (I also don’t have time to work out what the exact solution we need to use everywhere quite yet).

Further reading:

}}
"
:classicon="hasSparklineAnimation ? 'i-lucide:pause' : 'i-lucide:play'"
:aria-pressed="hasSparklineAnimation"
Copy link
Member

@knowler knowler Feb 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:aria-pressed="hasSparklineAnimation"

We don’t need to set aria-pressed since we’re already updating the label (and updating the label is generally better for this specific kind of button). From Sarah Higley’s ”Playing with state”:

As a final note, never change both the name and aria-pressed state in tandem. That way, confusion lies. Just imagine trying to parse the meaning of "play button, on" vs. "pause button, off".

@click="toggleSparklineAnimation"
>
<span class="sr-only">
{{
hasSparklineAnimation
? $t('package.trends.pause_animation')
: $t('package.trends.play_animation')
}}
</span>
</ButtonBase>
Comment on lines +319 to 340
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Find ButtonBase component
fd -i "buttonbase" --type f

Repository: npmx-dev/npmx.dev

Length of output: 43

🏁 Script executed:

# Check for ButtonBase definition in components
find app/components -name "*Button*" -type f | head -20

Repository: 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 -80

Repository: 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 2

Repository: npmx-dev/npmx.dev

Length of output: 1271

🏁 Script executed:

# Search for ButtonBase component definition across entire codebase
rg "ButtonBase" --type-list | head -5

Repository: 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 -100

Repository: 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 -20

Repository: npmx-dev/npmx.dev

Length of output: 1742

🏁 Script executed:

# Search for ButtonBase.vue in subdirectories
find app/components -name "*[Bb]utton*" -type f

Repository: 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 -20

Repository: npmx-dev/npmx.dev

Length of output: 88

🏁 Script executed:

# Check if ButtonBase is from external package
rg "ButtonBase" package.json

Repository: 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 -20

Repository: 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 -5

Repository: npmx-dev/npmx.dev

Length of output: 99

🏁 Script executed:

# Look for component definitions (defineComponent, <script setup>)
rg "name.*ButtonBase|component.*ButtonBase" -i

Repository: npmx-dev/npmx.dev

Length of output: 388

🏁 Script executed:

# Check app/components directory structure
find app/components -type d | head -30

Repository: 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.vue

Repository: 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.vue

Repository: npmx-dev/npmx.dev

Length of output: 129

🏁 Script executed:

# Check Button/Base.vue
cat app/components/Button/Base.vue

Repository: npmx-dev/npmx.dev

Length of output: 2078

🏁 Script executed:

# Verify how ButtonBase is referenced - might be aliased
rg "ButtonBase" --no-filename | head -5

Repository: 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.css

Repository: 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.vue

Repository: 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 -20

Repository: 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 -40

Repository: 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 -50

Repository: 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 -30

Repository: 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 -100

Repository: 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 -30

Repository: 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 -l

Repository: 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 -30

Repository: 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>

</div>
</template>
Expand Down
Loading