This started out as a quick follow-up to Daniel’s request for touch-ups on Zulip.
What this patch changes:
- use monospaced font for repo id
- add explanatory tooltips on hover of potentially unclear elements on repo card UI. All copies are in Title Case (Wikipedia style) and are designed to be glanceable and succinct.
- when hovering the seeder count badge: “$x Nodes Seeding This Repository”
- when hovering the issue/patch count: “$x Issues and $y Patches Open”
- when hovering the last-updated copy: “$fullTs — Latest Commit to Canonical Branch” (added trailing copy to pre-existing tooltip)
- when hovering the abbreviated RID: “$fullRid — Repository Identifier” (added trailing copy to pre-existing tooltip)
- remove some misleading inert styling code
- make seeder count badge less contrasting/distracting. This results in the eye having half as many scan targets per card row (just the repo title, not it and the seeder count badge).
- make seeder count badge blend better with card hover effect (previously unaffected resulting in the badge standing out)
- replace some divs with more semantic html
- clearly show when the card is keyboard-focused
- make badges not tabbable. Were so by default, even if they are not clickable (!) and in fact that functionality was nowhere used throughout the codebase.
Notable non-changes:
- I didn’t make the RID on the card clickable to copy the full RID since I think
- this will more often end up being clicked by mistake while users just want to click the card itself
- there’s a more intuitive alternative: click the card, the full RID is now front and center and able to be click-to-copied
This started out as a quick follow-up to Daniel’s request for touch-ups on Zulip.
What this patch changes:
- use monospaced font for repo id
- add explanatory tooltips on hover of potentially unclear elements on repo card UI. All copies are in Title Case (Wikipedia style) and are designed to be glanceable and succinct.
- when hovering the seeder count badge: “$x Nodes Seeding This Repository”
- when hovering the issue/patch count: “$x Issues and $y Patches Open”
- when hovering the last-updated copy: “$fullTs — Latest Commit to Canonical Branch” (added trailing copy to pre-existing tooltip)
- when hovering the abbreviated RID: “$fullRid — Repository Identifier” (added trailing copy to pre-existing tooltip)
- remove some misleading inert styling code
- make seeder count badge less contrasting/distracting. This results in the eye having half as many scan targets per card row (just the repo title, not it and the seeder count badge).
- make seeder count badge blend better with card hover effect (previously unaffected resulting in the badge standing out)
- replace some divs with more semantic html
- clearly show when the card is keyboard-focused
- make badges not tabbable. Were so by default, even if they are not clickable (!) and in fact that functionality was nowhere used throughout the codebase.
Notable non-changes:
- I didn’t make the RID on the card clickable to copy the full RID since I think
- this will more often end up being clicked by mistake while users just want to click the card itself
- there’s a more intuitive alternative: click the card, the full RID is now front and center and able to be click-to-copied
All copies are in Title Case (Wikipedia style) and are designed to be glanceable and succinct.
We don’t use title case anywhere else in the code. I suggest we stick to the code base convention in this patch to not mix how we do things. If we decide to change the convention then we should do it across the whole code base.
replace some divs with more semantic html
Same for this. Let’s first discuss this and then apply across the whole codebase.
filter: opacity(0.9);
Could we use one of the colors from our color system here instead of changing the opacity?
The rest LGTM. Just a note: radicle-explorer is in maintenance mode, so we try to always prioritize work on the desktop app.
Ad copy conventions, I understand that I shouldn’t use the Wikipedia-style Title Case but you haven’t made clear what I should use instead. Please provide a the desired copy verbatim as desired ensuring that after a copy-paste I’ll meet your existing copy convension.
Ad semantic html, “applying semantic HTML acrross the codebase” is a near impossible feat. I’ve done it in projects and it was months of work and patches with tens of thousands of lines changed. Also very very fragile procedure. Moreover I don’t see why it’s necessary to be done across the codebase atomically or in any kind of allignment. Semantic HTML is a standard, as long as we improve high traffic components now and then (e.g. whenever we touch something for whatever reason, improve it a bit) we’ll eventually get somewhere, otherwise it will never be a priority and never get done, for the same reason it wasn’t already done. If you still would prefer me to undo all such changes, then please let me know.
Ad coloring, I used opacity on purpose, specifically to avoid hardcoding any one-off colors, since it’s a relative one (relative colors are a good thing, no?). Opacity (and the more modern color-mix) are the way to go to blend colors in such cases, since blending (i.e. not standing out) is the desired effect here. I looked in the colore palette and didn’t find a fitting option, if I missed one please point it out to me. IMO the proper fix here would be neither what was already implemented, nor what I’m doing with opacity, but instead toggle visibility of a relatively full width & height contained layer overlaying the whole card, cleanly applying the shimmer effect to the whole card. I was going for that but eventually opted the opacity alternative as it touches the existing code and design the least.
Rolling back to only the first commit, using monospaced font for RID.
Changed a few small things to avoid another round-trip on this small issue.
-
we use CSS vars for all typography, this way switching the code font in Settings works across the app
-
extracted the inline styles into a class, we usually do this when there’s more than a handful of styles or we reuse them somewhere else, not super strict about this
-
we prefer using the Svelte
style:attributes over “style=”
👉 Preview
👉 Workflow runs
👉 Branch on GitHub
The e2e tests also seem to fail on master, will have to look into it separately. Visual spec failures are expected.
Re a11y, I liked the improvement on tabbing - ignoring focusing on badges and highlighting the cards. But I think it would be better to do a pass on a11y separately when we have more time for this and then do it across the whole app, so that users don’t get confused of why it’s working on the front page but not elsewhere.
The semantic html I’m not convinced about. In my limited experience I’ve always had trouble to chose which html tags to pick for which occasion, leading to inconsistent usage. Also nesting components can lead to the semanticness of it not making much sense. So I’d rather we not use them.
The color system def could be improved, at the time when we implemented it color-mix wasn’t widely supported, if I’d do it from scratch now, I’d def use it. But since the design system is in place now, I’d rather we stick to what we got and not mix in a new way of doing things to keep things consitent and easy to reason about (at least for now from the maintainer’s side).