feat(ui): use icon-only when connected to both services #1223
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! |
|
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:
📝 WalkthroughWalkthroughThis PR refactors the AccountMenu component by removing the Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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/Header/AccountMenu.client.vue (1)
68-76: Consider adding accessible text for connection status.The green dot visually indicates that the user is connected to services, but screen reader users have no equivalent indication. Consider adding visually hidden text to convey this status.
Proposed fix
<div v-if="hasAnyConnection" class="flex items-center relative"> <span class="w-6 h-6 rounded-full bg-bg-muted ring-2 ring-bg flex items-center justify-center" :class="hasBothConnections ? 'relative z-10' : ''" > - <span class="size-1.5 rounded-full bg-green-600 absolute top-0 -inset-ie-0"></span> + <span class="size-1.5 rounded-full bg-green-600 absolute top-0 -inset-ie-0" aria-hidden="true"></span> <span class="i-carbon-cloud w-3 h-3 text-fg-muted" aria-hidden="true" /> </span> + <span class="sr-only">{{ $t('account_menu.connected') }}</span> </div>This requires adding a
connectedtranslation key to your i18n files.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Header/AccountMenu.client.vue (1)
60-79:⚠️ Potential issue | 🟡 MinorConnection status is not accessible to screen readers.
The green dot indicator conveys the connected state visually but provides no accessible alternative. Screen reader users cannot perceive this status change. Consider adding an
aria-labelto the button that reflects the current connection state.Suggested fix
<ButtonBase type="button" :aria-expanded="isOpen" aria-haspopup="true" + :aria-label="hasAnyConnection ? $t('account_menu.connected') : $t('account_menu.connect')" `@click`="isOpen = !isOpen" class="border-none" >This assumes the appropriate translation keys exist. Adjust key names as needed.
🧹 Nitpick comments (1)
app/components/Header/AccountMenu.client.vue (1)
69-75: Consider removing vestigialz-10conditional class.The conditional
hasBothConnections ? 'relative z-10' : ''appears to be leftover from the previous multi-avatar stacking design. Since the UI now renders a single cloud icon regardless of how many services are connected, the z-index logic no longer serves a purpose. The parentdivat line 68 already provides therelativepositioning context needed for the status dot.Suggested simplification
<span class="w-6 h-6 rounded-full bg-bg-muted ring-2 ring-bg flex items-center justify-center" - :class="hasBothConnections ? 'relative z-10' : ''" >
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
|
|
what about instead displaying the logo of the connected service? (npm, bluesky or atproto) and we revisit when we get lots more... (could display a |
|
That could indeed be a valid alternative. I will use the logos |
How do you pick what logo to display though, if the user is connected to more than one thing? (assuming they can be) Could also have overlapping avatars. (But then again it gets tricky - which one do we show on top?) |
|
There's also an "issue" with the ATProto icon: it kinda looks like I want to connect my email. 
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Header/AccountMenu.client.vue (1)
60-88:⚠️ Potential issue | 🟠 MajorAdd an explicit accessible label for the icon-only button.
When connected, the button contains only aria-hidden icons, so it has no accessible name. Please add an
aria-label(you already addedaccount_menu.connectedin i18n).✅ Proposed fix
<ButtonBase type="button" :aria-expanded="isOpen" aria-haspopup="true" + :aria-label="hasAnyConnection ? $t('account_menu.connected') : $t('account_menu.connect')" `@click`="isOpen = !isOpen" class="border-none" >
|
@danielroe I've updated the PR with icons. |
|
we need to update the mobile menu as well: 
I think the icon looks good. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Header/MobileMenu.client.vue (1)
125-135:⚠️ Potential issue | 🟡 MinorMissing green status indicator for visual consistency.
The npm CLI connection (line 121) displays a green status dot to indicate active connection, but the Atmosphere connection lacks this indicator. For consistent UX, consider adding the same visual cue here.
💡 Suggested fix to add status indicator
<span class="flex-1 truncate">@{{ atprotoUser.handle }}</span> + <span class="w-2 h-2 rounded-full bg-green-500" aria-hidden="true" /> </button>
| isConnected: isNpmConnected, | ||
| isConnecting: isNpmConnecting, | ||
| npmUser, | ||
| avatar: npmAvatar, |
There was a problem hiding this comment.
still wondering if it makes sense to show avatars in the dropdown, even if not in the top level .... 🤔
wdyt?
There was a problem hiding this comment.
I think it may look a bit confusing and inconsistent.
If I have to go with avatars, which I do like because they give the impression that I've actually connected, I prefer the initial proposal.
|
Personal opinion, I want to see my face if I am just logged in via Bluesky. So maybe for the first service, you show the avatar, and for subsequent, you do the other thing. This is my preference:
e.g. If you're logged into Bluesky only, show the Bluesky avatar in the header. If you're logged into both, show the FIRST ONE in the list in the header, which at this point is Bluesky. Additionally, the list items of available services should be in the same order all the time to avoid confusion. e.g. Bluesky first |
MatteoGabriele
force-pushed
the
feat/use-icon-when-connected
branch
from
6ce8814 to
e5f60f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Header/AccountMenu.client.vue (1)
64-88:⚠️ Potential issue | 🟠 MajorAdd an accessible label for the icon-only account button.
When connected but no avatar is available, the button renders only anaria-hiddenicon, leaving no accessible name for screen readers. Add a hidden label (or anaria-label) so the menu remains discoverable.Proposed fix
<span v-if="hasAnyConnection" class="flex items-center"> <img v-if="isNpmConnected && npmAvatar" :src="npmAvatar" :alt="npmUser || $t('account_menu.npm_cli')" width="24" height="24" class="w-6 h-6 rounded-full ring-2 ring-bg object-cover" /> <img v-else-if="atprotoUser?.avatar" :src="atprotoUser.avatar" :alt="atprotoUser.handle" width="24" height="24" class="w-6 h-6 rounded-full ring-2 ring-bg object-cover" /> <span v-else class="w-6 h-6 rounded-full bg-bg-muted ring-2 ring-bg flex items-center justify-center" > <span class="i-carbon-cloud w-3 h-3 text-fg-muted" aria-hidden="true" /> </span> + <span class="sr-only"> + {{ npmUser || atprotoUser?.handle || $t('account_menu.connect') }} + </span> </span>
| <button | ||
| v-else | ||
| type="button" | ||
| role="menuitem" | ||
| class="w-full px-3 py-2.5 flex items-center gap-3 hover:bg-bg-muted transition-colors text-start rounded-md" | ||
| @click="openConnectorModal" | ||
| > | ||
| <span class="w-8 h-8 rounded-full bg-bg-muted flex items-center justify-center"> | ||
| <span | ||
| v-if="isNpmConnecting" | ||
| class="i-carbon-circle-dash w-4 h-4 text-yellow-500 animate-spin" | ||
| aria-hidden="true" | ||
| /> | ||
| <span v-else class="i-carbon-terminal w-4 h-4 text-fg-muted" aria-hidden="true" /> | ||
| </span> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for UnoCSS icon class usage patterns in Vue components
rg 'class=".*i-carbon' app/components --type vue -A 1 -B 1 | head -50Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
# Specifically check AccountMenu.client.vue for icon usage
cat -n app/components/Header/AccountMenu.client.vue | grep -E '(i-carbon|class=)' | head -20Repository: npmx-dev/npmx.dev
Length of output: 1661
🏁 Script executed:
# Check for both colon and dash syntax usage across the codebase
rg 'i-carbon[:|-]' app/ -o | sort | uniq -cRepository: npmx-dev/npmx.dev
Length of output: 4817
Use UnoCSS colon syntax for the Carbon icon classes.
The colon form provides better performance for icons with dashes. Use i-carbon:circle-dash and i-carbon:terminal instead of the dash-separated form.
Proposed fix
- <span
- v-if="isNpmConnecting"
- class="i-carbon-circle-dash w-4 h-4 text-yellow-500 animate-spin"
- aria-hidden="true"
- />
- <span v-else class="i-carbon-terminal w-4 h-4 text-fg-muted" aria-hidden="true" />
+ <span
+ v-if="isNpmConnecting"
+ class="i-carbon:circle-dash w-4 h-4 text-yellow-500 animate-spin"
+ aria-hidden="true"
+ />
+ <span v-else class="i-carbon:terminal w-4 h-4 text-fg-muted" aria-hidden="true" />📝 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.
| <button | |
| v-else | |
| type="button" | |
| role="menuitem" | |
| class="w-full px-3 py-2.5 flex items-center gap-3 hover:bg-bg-muted transition-colors text-start rounded-md" | |
| @click="openConnectorModal" | |
| > | |
| <span class="w-8 h-8 rounded-full bg-bg-muted flex items-center justify-center"> | |
| <span | |
| v-if="isNpmConnecting" | |
| class="i-carbon-circle-dash w-4 h-4 text-yellow-500 animate-spin" | |
| aria-hidden="true" | |
| /> | |
| <span v-else class="i-carbon-terminal w-4 h-4 text-fg-muted" aria-hidden="true" /> | |
| </span> | |
| <button | |
| v-else | |
| type="button" | |
| role="menuitem" | |
| class="w-full px-3 py-2.5 flex items-center gap-3 hover:bg-bg-muted transition-colors text-start rounded-md" | |
| `@click`="openConnectorModal" | |
| > | |
| <span class="w-8 h-8 rounded-full bg-bg-muted flex items-center justify-center"> | |
| <span | |
| v-if="isNpmConnecting" | |
| class="i-carbon:circle-dash w-4 h-4 text-yellow-500 animate-spin" | |
| aria-hidden="true" | |
| /> | |
| <span v-else class="i-carbon:terminal w-4 h-4 text-fg-muted" aria-hidden="true" /> | |
| </span> |
|
@whitep4nth3r @danielroe based on both your ideas, I have made the changes when nothing is connected when atproto is connected when npm is connected when both are connected |
MatteoGabriele
changed the title
|
I think users are so used to seeing their own avatar in that spot, so I think we should have one there even if the user is connected to multiple services. The question then is - which one do we show? The first one the user connected? The most recent one? Always using the first one connected, as @whitep4nth3r suggested, does have the benefit of being less of a surprise, and potentially confusing when the avatar they're used to happens to change just because they connected some other service |
|
@Tobbe I do agree with you. I proposed using the same connected icon (with a green dot) for every service to maintain consistency. People usually see their own avatar, but they rarely expect to see all avatars piled together when connected to multiple services. Maybe we could do progressive enhancement. |
I agree. That's why I suggested just showing one of them in the menu toggle button. I'm fine with going with Salma's suggestion of always just using the avatar from the service the user first connected. |
|
How do we know which one is picked first tho? We probably need to save it somewhere, right? 🤔 |
We don't already store the timestamp for when a service was connected, do we? |
4 participants
Adding avatars for each connected service has several problems:
My proposal is to retain only the single cloud icon and add a green dot to indicate to the user that it is connected.
before
after
