fix(a11y, ui): make copy commands buttons always visible#998
fix(a11y, ui): make copy commands buttons always visible#998IdrisGit wants to merge 9 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCopy buttons in app/components/Terminal/Execute.vue, app/components/Terminal/Install.vue and app/components/Package/SkillsModal.vue were changed from hidden-by-default (opacity/group-hover) to persistently visible; their inner content was replaced with an icon that toggles between a copy icon and a checkmark based on copy state. Button border classes were standardised (including border-solid) and hover/focus styling simplified. test/e2e/create-command.spec.ts was updated to remove hover steps, assert persistent visibility of copy buttons, and add Run Command copy tests that verify clipboard contents and copy confirmation behaviour. 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)
No actionable comments were generated in the recent review. 🎉 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 (2)
app/components/Terminal/Execute.vue (1)
71-74: Drop per-button focus-visible utility; rely on the global button focus style.The inline
focus-visible:outline-accent/70on a<button>conflicts with the global focus-visible rule inapp/assets/main.css. Please remove it to keep focus styling consistent.Proposed change
- class="px-2 py-0.5 font-mono ms-auto text-xs text-fg-muted bg-bg-subtle/80 border border-border rounded transition-colors duration-200 hover:(text-fg border-border-hover) active:scale-95 focus-visible:outline-accent/70" + class="px-2 py-0.5 font-mono ms-auto text-xs text-fg-muted bg-bg-subtle/80 border border-border rounded transition-colors duration-200 hover:(text-fg border-border-hover) active:scale-95"Based on learnings: focus-visible styling for buttons and selects is applied globally via main.css; avoid per-element focus-visible utility classes like
focus-visible:outline-accent/70.app/components/Terminal/Install.vue (1)
124-127: Remove per-button focus-visible utilities and rely on the global rule.These three buttons still include
focus-visible:outline-accent/70, which conflicts with the project’s global focus-visible styling inapp/assets/main.css.Proposed change
- class="px-2 py-0.5 font-mono ms-auto text-xs text-fg-muted bg-bg-subtle/80 border border-border border-solid rounded transition-colors duration-200 hover:(text-fg border-border-hover) active:scale-95 focus-visible:outline-accent/70" + class="px-2 py-0.5 font-mono ms-auto text-xs text-fg-muted bg-bg-subtle/80 border border-border border-solid rounded transition-colors duration-200 hover:(text-fg border-border-hover) active:scale-95"- class="px-2 py-0.5 font-mono ms-auto text-xs text-fg-muted bg-bg-subtle/80 border border-border border-solid rounded transition-colors duration-200 hover:(text-fg border-border-hover) active:scale-95 focus-visible:outline-accent/70" + class="px-2 py-0.5 font-mono ms-auto text-xs text-fg-muted bg-bg-subtle/80 border border-border border-solid rounded transition-colors duration-200 hover:(text-fg border-border-hover) active:scale-95"- class="px-2 py-0.5 font-mono ms-auto text-xs text-fg-muted bg-bg-subtle/80 border border-border border-solid rounded transition-colors duration-200 hover:(text-fg border-border-hover) active:scale-95 focus-visible:outline-accent/70" + class="px-2 py-0.5 font-mono ms-auto text-xs text-fg-muted bg-bg-subtle/80 border border-border border-solid rounded transition-colors duration-200 hover:(text-fg border-border-hover) active:scale-95"Based on learnings: focus-visible styling for buttons and selects is applied globally via main.css; avoid per-element focus-visible utility classes like
focus-visible:outline-accent/70.Also applies to: 186-189, 231-233
|
I like it. I just feel the eye has to travel from the button to its related content. How could this be mitigated ? |
@graphieros fair point, I think quick way would be to remove the right align and show the button in it's original place, later we can think about a better position or if you have any ideas. 
|
|
I think we need to know what others think. The initial intention was to avoid cluttering the content. |
|
@graphieros yeah that makes sense, I will try with icons and post screenshots here |
IdrisGit
force-pushed
the
fix-copy-button-visibility
branch
from
a69fdd0 to
905a187
Compare
|
Ok after trying few things and researching other platforms:
|
IdrisGit
force-pushed
the
fix-copy-button-visibility
branch
from
905a187 to
17c4dd8
Compare
There was a problem hiding this comment.
I think this is a great change. I consider copy buttons like these to be added conveniences since users can already select and copy text, so I typically am a bit more lenient about only revealing them on hover/focus (as a compromise). We do this for copying the package name at the top of the page, since there’s already a lot of clutter. This case is a bit different because we do have the space.
With regard to clutter, I think we should find a way to better distinguish the button from the adjacent command’s text. Perhaps thicker borders or inverting the background/foreground colours.
As well, I do not think we should right align them. This actually reduces the usability/discoverability for some users as they have trace across a bunch of empty space (i.e. a physical and/or cognitive challenge) or they might not notices they’re there (e.g. magnifier users or folks with visual impairments like tunnel vision).
IdrisGit
force-pushed
the
fix-copy-button-visibility
branch
from
17c4dd8 to
97c7c56
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Terminal/Install.vue (1)
186-192:⚠️ Potential issue | 🟡 MinorAdd missing accessibility attributes for consistency.
This button is missing the
aria-labelattribute andaria-live="polite"wrapper that the install command (line 127-130) and create command (line 234-239) buttons have. For a PR focused on accessibility, these should be consistent.Suggested fix
<button type="button" - class="px-2 py-0.5 font-mono ms-auto text-xs text-fg-muted bg-bg-subtle/80 border border-border border-solid rounded transition-colors duration-200 hover:(text-fg border-border-hover) active:scale-95 focus-visible:outline-accent/70" + class="px-2 py-0.5 font-mono ms-auto text-xs text-fg-muted bg-bg-subtle/80 border border-border border-solid rounded transition-colors duration-200 hover:(text-fg border-border-hover) active:scale-95" + :aria-label="$t('package.get_started.copy_command')" `@click.stop`="copyRunCommand(executableInfo?.primaryCommand)" > - {{ runCopied ? $t('common.copied') : $t('common.copy') }} + <span aria-live="polite">{{ runCopied ? $t('common.copied') : $t('common.copy') }}</span> </button>
🧹 Nitpick comments (3)
app/components/Terminal/Execute.vue (1)
71-78: Remove inlinefocus-visible:outline-accent/70utility; rely on global styling.Per project guidelines, focus-visible styling for buttons is handled globally in
main.css. The inlinefocus-visible:outline-accent/70class should be removed.Additionally, for consistency with the Install.vue buttons:
- Add
border-solidto match the standardised border styling- Consider wrapping the button text in
<span aria-live="polite">so screen readers announce the "Copied" state changeSuggested fix
<button type="button" - class="px-2 py-0.5 font-mono ms-auto text-xs text-fg-muted bg-bg-subtle/80 border border-border rounded transition-colors duration-200 hover:(text-fg border-border-hover) active:scale-95 focus-visible:outline-accent/70" + class="px-2 py-0.5 font-mono ms-auto text-xs text-fg-muted bg-bg-subtle/80 border border-border border-solid rounded transition-colors duration-200 hover:(text-fg border-border-hover) active:scale-95" :aria-label="$t('package.get_started.copy_command')" `@click.stop`="copyExecuteCommand" > - {{ executeCopied ? $t('common.copied') : $t('common.copy') }} + <span aria-live="polite">{{ executeCopied ? $t('common.copied') : $t('common.copy') }}</span> </button>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… Do not apply per-element inline utility classes like focus-visible:outline-accent/70 on these elements."
app/components/Terminal/Install.vue (2)
124-131: Remove inlinefocus-visible:outline-accent/70utility.Per project guidelines, focus-visible styling for buttons is handled globally in
main.css. Remove the inline utility class.Suggested fix
<button type="button" - class="px-2 py-0.5 font-mono ms-auto text-xs text-fg-muted bg-bg-subtle/80 border border-border border-solid rounded transition-colors duration-200 hover:(text-fg border-border-hover) active:scale-95 focus-visible:outline-accent/70" + class="px-2 py-0.5 font-mono ms-auto text-xs text-fg-muted bg-bg-subtle/80 border border-border border-solid rounded transition-colors duration-200 hover:(text-fg border-border-hover) active:scale-95" :aria-label="$t('package.get_started.copy_command')" `@click.stop`="copyInstallCommand" >Based on learnings: "In the npmx.dev project… Do not apply per-element inline utility classes like focus-visible:outline-accent/70 on these elements."
231-240: Same issue: remove inlinefocus-visible:outline-accent/70.As with the other buttons, remove the inline focus-visible utility class to rely on the global styling.
Suggested fix
<button type="button" - class="px-2 py-0.5 font-mono ms-auto text-xs text-fg-muted bg-bg-subtle/80 border border-border border-solid rounded transition-colors duration-200 hover:(text-fg border-border-hover) active:scale-95 focus-visible:outline-accent/70" + class="px-2 py-0.5 font-mono ms-auto text-xs text-fg-muted bg-bg-subtle/80 border border-border border-solid rounded transition-colors duration-200 hover:(text-fg border-border-hover) active:scale-95" :aria-label="$t('package.create.copy_command')" `@click.stop`="copyCreateCommand" >
|
Here is one way we can do it, keeps the button near the command, no surprise on hover and uses existing copy icons for familiarity 
|
|
I think this looks very nice |
|
@graphieros I have updated the PR to the above screenshot, please review and let me know your thoughts |
There was a problem hiding this comment.
The design is good. We can always add the visual label later if we think it would help.
We just seem to be missing labels for the copy buttons for the “Run locally” commands.
Also, I’m noticing with the change to just an icon, now we don’t have the live regions to indicate the commands were copied. It would be good if we could keep the live region even if it is visually hidden text.
I have added the missing aria label to the run command.
I didn't get this part, it doesn't have some sort of feedback on copy or do you mean dynamic aria lables? Currently the icon turns from carbon/copy to checkmark when user copies the command. copy_commands_compressed.mp4 |
3 participants
There are two issues I am trying to solve in this PR:
I was confused where the copy button was and when I decided to highlight using cursor and copy manually that I saw the copy button popup.
screen recording of the issue and the fix
copy_button_compressed.mp4
side note: as a side effect it also fixes an issue on mobile where it's very hard to know where the copy command, since the opacity of the button changes based on the hover state.