Story 2212: Contributors List Component#2268
Conversation
| {% if author.profile_url %} | ||
| <a href="{{ author.profile_url }}" class="user-profile__avatar-link"> | ||
| {% include "v3/includes/_avatar_v3.html" with src=author.avatar_url name=author.name size="xl" %} | ||
| {% include "v3/includes/_avatar_v3.html" with src=author.avatar_url name=author.name size="xl" only %} |
There was a problem hiding this comment.
Nice. Were you seeing a context leak, or is this just proactive? Should we be doing this other places we pass props to components?
There was a problem hiding this comment.
I did get a context leak, since I was also use a variant keyword in my component (avatar has a variant input). We should proactively be using only when using other components.
There was a problem hiding this comment.
Something to share with the team!
herzog0
left a comment
herzog0
left a comment
There was a problem hiding this comment.
Looking pretty good!
I'm pre-approving, with a small ask: there seems to be missing a divider below the "All contributors" header in the mobile version.
Figma:
Good catch, this was a subtle issue caused by the hr not having a min-height, so when we set a max height on the card it got squished out of existence. |
jlchilders11
force-pushed
the
jc/contributors-list
branch
from
88e320d to
b8f93a8
Compare
julhoang
left a comment
julhoang
left a comment
There was a problem hiding this comment.
This LGTM! I'm pre-approving with 1 tiny request to update the semantic tag for the card title to use heading. Thanks in advance!
| {% endcomment %} | ||
| <div class="basic-card card py-large contributors-list {% if variant == 'all' %} all-contributors {% endif %}"> | ||
| <div class="card__header"> | ||
| <span class="card__title">{{ title }}</span> |
There was a problem hiding this comment.
Would you mind updating this to <h2>?
There was a problem hiding this comment.
Can we semantically guarantee that this will always be an h2 when it is included? Just thinking in terms of including it on a page, it might exist as part of a section that already has an h2 or h3
There was a problem hiding this comment.
Based on our current page designs, I think it's very likely to always be <h2> . Alternatively, we can set h2 as the default and add a new optional parameter for the consumer to set a different heading level.
There was a problem hiding this comment.
I added this semanted header selection functionality. Do we think this is something we may use on other cards enough to spin off into its own include?
| {% if heading_level == '2' or not heading_level %} | ||
| <h2 class="card__title">{{ title }}</h2> | ||
| {% elif heading_level == '3' %} | ||
| <h3 class="card__title">{{ title }}</h3> | ||
| {% elif heading_level == '4' %} | ||
| <h4 class="card__title">{{ title }}</h4> | ||
| {% elif heading_level == '5' %} | ||
| <h5 class="card__title">{{ title }}</h5> | ||
| {% elif heading_level == '6' %} | ||
| <h6 class="card__title">{{ title }}</h6> | ||
| {% endif %} |
There was a problem hiding this comment.
Just a thought – looking at this I feel like we should probably eventually have a card heading component to handle these heading level mapping and use it in other cards as well. Thanks for this update!
There was a problem hiding this comment.
Great minds think alike, I commented the same on the other comment chain.
There was a problem hiding this comment.
Ooops sorry I missed that, I'm glad we're on the same page 😆🙌. This should be used in other cards as well
gregjkal
left a comment
gregjkal
left a comment
There was a problem hiding this comment.
Couple more small items
4 participants
Issue: #2212
Summary & Context
contributor-list.cssto style this list across different breakpointsCloses #2212
Changes
v3/inclues/_contributors_list.htmlstatic/css/v3/contributors-list.cssPlease list any potential risks or areas that need extra attention during review/testing
None
Screenshots
Release:
All:
Self-review Checklist
Frontend