Conversation
julhoang
left a comment
julhoang
left a comment
There was a problem hiding this comment.
Overall this looks great! I just left 2 comments about text placement + Badge component variant
julhoang
left a comment
julhoang
left a comment
There was a problem hiding this comment.
Hi @jlchilders11 , this is looking great!! I left a few suggestions on using more precise semantic HTML elements for the text, plus a nit UI bug with max-width not being properly applied.
| Achievements - Optional: A list of achievements, each of which has points, a title, and a description | ||
| {% endcomment %} | ||
|
|
||
| <div class="wide-card card py-large"> |
There was a problem hiding this comment.
I can see you add a max-width for the card, but it's currently not being applied properly since card is overriding wide-card – we just need to swap their order here to card wide-card py-large 😄
There was a problem hiding this comment.
This seems to work in either order in firefox, which is strange.
|
|
||
| <div class="wide-card card py-large"> | ||
| <div class="card__header"> | ||
| <span class="card__title">Achievements</span> |
There was a problem hiding this comment.
Let's change this to <h2> or <h3>
There was a problem hiding this comment.
While I am happy to make this change, adding headers into include components does make me nervous. Since we are unaware of the surrounding context when it is included, it can lead to missing or unnatural heading hierarchies which dings us on accessibility.
… jc/achievement-cards
…classes, use more appropriate html structures
julhoang
left a comment
julhoang
left a comment
There was a problem hiding this comment.
Thanks for the updates @jlchilders11 !! I just have 1 last request to update the font-size token since it doesn't exist 🙏
| .achievement-card__ach-text { | ||
| /* Sans/Desktop/Regular/XS/Default */ | ||
| font-family: var(--font-sans); | ||
| font-size: var(--size-xs); |
There was a problem hiding this comment.
This token doesn't exist and it should be --font-size-xs 😄
3 participants
Figma: https://www.figma.com/design/iMGM8rMhU8K8WXbfFMoBFO/UI-Kit---Foundations-Delivery?node-id=552-3036&m=dev
Light Mode:
Dark Mode:
Mobile:
