-
Notifications
You must be signed in to change notification settings - Fork 153
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
Add share buttons to blog pages #3383
Conversation
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.
This looks great @mmmavis! I only caught two things:
Overall
Is it possible to have the side sticky social buttons disappear once the bottom social buttons appear? Feels redundant to show both.
Tablet:
Buttons should fit in one line horizontally
If we don't wanna stack the buttons on tablet... are we okay with letting the buttons wrap themselves based on its container width? This means on some screens the buttons might show in 2 rows, with the first row having 3 buttons and second row having the last button. |
Hmm ... it wouldn't be ideal. Looking at the screen again the left aligned design isn't actually working that well. I took a look at how we handled the social buttons on PNI and was wondering how big of a lift would it be to have the bottom social buttons behave like the creepometer share buttons? 😬 |
(Sorry for the lengthy reply. We can hop on a call to chat about this as well) @beccaklam from #3321, #3322, #3323, and PR #3352 (and now landed on staging style guide the "share button group" section) I'm assuming our goal is to have one share button "group" component that can work across the site (both functionalities and design). Buttons on PNI aren't really following specs in #3321, #3322, #3323 so I assume they should on the to-update list. We need to decide whether we want to unify social button styling from the "button" level or "button group" level and update the following tickets accordingly.
Having a shared button group component makes future implementation and design a lot easier as the code and style is set so we don't have to worry about devs/designers accidentally creating a new version of share button group. However, I do understand that it is challenging to design something that can look good for every use case... If we want to adjust button group style case by case then we shouldn't design it from "button group" level as this means we will end up having styling overrides everywhere. Do you think we should loop others in for this? Whatever we decide we just need to be consistent about it so we don't end up going back to where we were before 😂 (aka having multiple versions of share button (group) styles on the site). Don't worry about if I have to redo the implementation or not, I rather fix things early than late 😉 . (If I understand Sketch's symbols correctly) |
Hey @mmmavis no worries about the lengthy reply at all. I see what you mean, and knew that I was opening a larger conversation by asking about the PNI styling. I think currently the design I gave you may not be flexible enough and think perhaps we should go with something more responsive. And yes we should pull others in ... maybe we can talk about it Monday after stand? |
Hi @mmmavis so based on our conversation today I would say 113px wide would be the min width for the single row buttons. That is how wide the button would be for Facebook (the longest word) with 8px spacing between the button, icon, and the label: |
@beccaklam the minimum width for the Facebook button seems to be roughly
|
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.
This looks great, @mmmavis ! 👍 💯 Thank you!
@Pomax this is ready for code review! 😃 |
Code looks good! @mmmavis @beccaklam do we want to make the buttons fade out/in rather than abruptly existing, though? It might be jarring to readers to have them suddenly exist or be gone? |
Sounds good but I'll let @beccaklam decide what kind of effect we want to have. I'll file a followup ticket and tag you both soon 😄 |
Closes #3157
Test pages
Sticky side button group is set to be
140px
from the nav so the button group can align with the blog title. Depending on how much content is after the blog body... in some cases the sticky side button group can reach to the bottom (and align with the last element in the blog body) and in some other cases it won't align.when "after blog body" content is being long
when "after blog body" content is being short