-
Notifications
You must be signed in to change notification settings - Fork 102
feat(post-summary): update to SHINE styles #2137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: beta
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 85d29f8 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 processing.
|
👷 Deploy Preview for stacks-svelte processing.
|
giamir
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I skimmed through it and everything seems in order from a code perspective @dancormier. Thanks for tackling this huge component.
The only thing I observed is that for the Svelte component the readableTimestamp tooltip is somehow not tabbable via keyboard.
| &&__minimal { | ||
| .s-post-summary--content { | ||
| width: 100%; | ||
| @psx-container-sm: 28rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curiosity: How did we settle on 448px as a breakpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose it after some trial and error. I figured we needed a breakpoint that was larger than the sidebar width (21.5rem, 344px). I tried that but felt like it looked pretty cluttered at just above that breakpoint. This image is at with the container right at 345px with the breakpoint at 344px:
For that reason, I chose the next defined size up in our library (the 28rem). That said, I could be convinced that this breakpoint should match up with the sidebar size. Any strong opinions on that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me, it is just not one of our "official" breakpoints meaning that it will change the layout at a custom viewport size compared to other things. I guess it is fine. I wonder though if the sm breakpoint (780px one) could have suffice. In the sense that for viewports below that size we show the compact version even if in theory there is the estate to show more.
Anyway I am good with what we have. This is really a NIT. Thanks Dan.
CGuindon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YAY!!! Looking good! I only have small nit picky stuff:
- I noticed the line-height on the description and title are 18px — which feels a bit tight. We increased the base line-height to 140% which should put the line-height of the description to around 19.6px (we probably rounded up to 20px as that's what I'm seeing on the typography page for Body1). I think we had the line-height not centralized before so wondering if this is a case if it inheriting the custom old vs new here.
-
I'm seeing a 6px gutter used as spacing in between each line. Unfortunately (sorry), the spacing here isn't all the same. We have 8px, 4px and 10px amounts between each line. (Note the bottom tags container does have a sneaky 2px margin top if that helps or not with the math).
-
There should be an 8px gap between each tag (instead of 4).
-
The gutter between the score box and the container with the answer data should be 8px (instead of 6).
-
The font and icon color for the answer data (when not accepted) should be
black-400
- Similar to how you have the classes listed out at the top, I think it might be worth listing out all the class options above each example (within each section). For example, showing the
truncateclasses and the state classes (danger, warning, etc) all listed out in a table between description and code example. I found it a bit hard sometimes to see find the "different" classes within the code examples that show everything.
- Border left on answers should be
black-200(instead of 150)
Co-authored-by: Giamir Buoncristiani <gbuoncristiani@stackoverflow.com>
|
Thanks for the review @CGuindon! I've address everything you mentioned. Let me know if I missed anything! |
|
@dancormier Sorry I probably miscommunicate the line-height. It looks good on the description now but still needs that 140% on the title of the post as well. Should be 25px based on our new sizing. Looks like you have 20px for both |
|
@CGuindon sorry for the oversight! I've updated the title to have a 25px (aka 1.563rem) line height. |
CGuindon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the Answer icon missing (should be released now in the icons package) but otherwise LGTM 🙌
CGuindon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌
mukunku
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow this is amazing stuff. Great work, Dan!
Left one small comment below but otherwise flawless 🤌
| {% else %} | ||
| {% icon "Answer16" %} | ||
| {% endif %} | ||
| {{ data.answers | default: 1 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Update post summary visual test regression images * Update test images



SPARK-75, Figma
This PR updates the Post Summary to the latest SHINE styles. cc @ycantu
Note
See also this PR which includes updated visual regression test images.
Deploy previews
TODO
8px: Usercard/stats to title4px: Title to excerpt10px: Excerpt to tags8px8pxblack-400v-truncateclass tableblack-200Open questions/requests
:visitedtitle given that we cannot manipulate thefont-weightproperty in this context?Answer16Answer16FillShield(using legacy icon for now)