feat(user card) - Update User card to new SHINE design (part 1) #2092
Conversation
🦋 Changeset detectedLatest commit: 36a9d83 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for stacks ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for stacks-svelte ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hey @ttaylor-stack , I did a quick glance through the code and it overall looked good to me. Looking at the UI elements I noticed an issue with gaps, though: Here's what I see in the designs: |
There was a problem hiding this comment.
Base should be using the 24x24 avatar size.
The padding in between the bling and the badge/rep number is created with a space character before the number. Should this be done with padding/gutter spacing instead? Would be 2px.
Is there any way to have 4px spacing in between badges when the count is showing vs 2px spacing when the count number isn't?
For responsive, when we need to wrap, I think we should keep certain blocks of info together (so the whole block wraps when the screen size is too short)
Got confused by my own section title — can we change this to "with user badges"?
OR — I'm wondering if we should also have a "with Bling" section OR should both of those just be in the Base section?
Should / could the Sizes be in the same section? Maybe in a table to get both descriptions or just the descriptions above separate by a paragraph and both sizes in the same code box?
There was a problem hiding this comment.
- The gutter space between the avatar image and the username should be 6px — looks like it's using the same value as the gutter spacing in between the bling/rep — that one can remain 4px (currently 4px everywhere).
- Similarly, the gutter spacing before the asked {timestamp} should be 8px but the spacing in between username and rep/bling is 6px (currently 6px everywhere).
- rep # should be
black-600text color (using 500)
-
In the example, we show that the username is a link — the mouse changes the cursor to the pointer when I hover and it's a dead link when I click. The timestamp "asked 2h ago" would also be a link in production. Could we/should we show this in the code example as well? Have it be a similar dead link but the cursor changes on hover.
-
Now that I see it, I think we can remove the example with the mod badge from the Base section. We have the "With Badge" right below where we show those variants. Similarly, we'll have "With bling" in user card part two that will show the trophy variation too down below.
- Nice work on the responsive breakdown of all the pieces. There is the odd user that likes to have a sentence in their username. This is more rare but wondering if you have any suggestions on if/how we could wrap the username if we need to (instead of it cutting off). This is lower priority
- For the large size, there's different sizes for all the padding in there (currently all 6px). Some should be 12px or 4px.
CGuindon
left a comment
CGuindon
left a comment
There was a problem hiding this comment.
Missed one small one, these gaps are both 6px. The left gap should be 6 but the right gap (before "asked 2h ago" should be 8px.
I had this note from before the holidays — it's lower priority since we won't have just the badges showing alone without the numbers for Beta but curious if this is an easy fix. If it takes too long, I can descope for now.
| white-space: var(--_uc-link-ws); | ||
| & &--time { | ||
| color: var(--black-400); | ||
| margin-left: var(--su2); |
There was a problem hiding this comment.
@ttaylor-stack we can't apply a left margin here because when it wraps, we'll wind up with an extra 2px gap to the left of the timestamp
@CGuindon I vote for descope since it would require some tricky CSS |
@CGuindon I think the gaps should be good now. See the screenshots in the toggle section below (my tool shows an extra 1px for each value. No clue why and I'm choosing to ignore that and just assume that the fact that the timestamp is +2px from the previous item is good enough 😂) Screenshots
|
giamir
left a comment
giamir
left a comment
There was a problem hiding this comment.
@ttaylor-stack and myself will go through few things on Monday regarding the Svelte API. Let's hold off merging for now.
Here are my review notes for our call on Monday
- It looks like the markup example between
defaultandwith badgesdon't differ in the docs. I think it is important that we try to map what we show in the example to the markup so that engineer can easily copy/paste what they need. - I am a bit confused about the
Note on timestampsguidance we provide. At the moment all the examples showasked .... I think it would be appropriate to show concrete example of what we meant with that guidance (e.g.modified ...or even the actual precise date in the tooltip). If we don't do this we risk to confuse engineers and then it would be better to remove that guidance all together. - Updating the migration guide
- Updating markup structure of visual and accessibility tests to reflect the new classes, etc...
- It looks like all the subcomponents created UserCardAwards, UserCardBadges, etc… are used only internally and we don't expose them, therefore their API should not be published in the story for consumers to see
Then for the UserCard.svelte api here are some preliminary thoughts that we will discuss together:
<UserCard>
Props
- name (string - required)
- avatar? (string - src to the user avatar)
- profileUrl? (string - url to the user profile page - previously called href)
- size? (sm/default/lg - default: default)
- reputation? (string | number - default: undefined)
- role? string // can be confusing | this is not an html attribute | find a different name
- location? string
// Badges (these are all badges, can we just create a badges snippet and give up control to the consumer? - create more flexibility)
admin?
moderator?
staff?
// Awards (Those are all blings, should we just have an awards snippet instead and let the consumer slot them instead of passing single props)
- goldAwards?
- silverAwards?
- bronzeAwards?
// Timestamp (extra child component??? - bad naming - maybe we should just call it UserCardTime like we do in the CSS class —time - would be placed in a time snippet)
- timestamp?
- i18nTimestampTooltip?
- timestampHref?
// Recognition (can it be a snippet? can we give up control here and and let the consumer take care of it)
text
href
linkText
Icon
// Can this just be a bio snippet
bio
### Coming up in the follow up tickets
original poster (probably a modifier class in CSS and could be a boolean prop for the svelte component - op and opTooltipText)
new (just another badge in the badges snippet - consumers take care of it)
deleted? (boolean - do we leave to the consumer to make sure the avatar prop is changed to deleted avatar or do we hardcode that specific avatar in the component?)
additional blings - could they just be a snippet for the consumer to fill? what level of control do we need?
|
Thank you very much for this review @giamir!
I think this is my mistake. I updated the example but not the markup example. I've removed the extra markup in the example in 36a9d83 The examples are still very similar, but I included both
This is also on me. I added that tip based on the callout in the Figma without scrutinizing it. Now that I really read it, I'm a bit confused by the intention, both in the docs and with the UI. I've asked a few questions directly on the Figma.
I will take care of adding the markup changes to the migration guide but I'll hold on anything beyond that.
➕
Seems reasonable. For
➕ to all of this 🙂
Agreed. For deleted, I vote we hardcode the deleted avatar and render it without the need for consumers to pass it. I broadly agree with these notes on the Svelte component. I think we should approach architecting these components with a heuristic that's something like:
I'd expect something like: // UserCard props
interface Props {
name: string;
avatar?: string;
bio?: Snippet;
designation?: string;
location?: string;
profileUrl?: string;
reputation?: string | number;
size?: Size;
// Expose subcomponents for these snippets
awards?: Snippet;
badges?: Snippet;
recognition?: Snippet;
time?: Snippet;
i18nReputationLabel?: string;
class?: string;
}I'm happy to help brainstorm on this more. Let me know what you think and feel free to schedule a call (or @ttaylor-stack would you like to add @giamir to our 1:1 on Monday?) |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This ticket cover part 1 of the user card updates.
The following things will be updated in Part 2:
Ticket
Figma
notes:
I decided to keep the Usercard as one Svelte component instead of dividing it up even though the large and sm/default sizes have different markup. I thought it would be more user friendly to have 1 components users interact with.
I decided split the different parts of the Usercard into different Svelte subcomponents