Skip to content
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

Merged
merged 10 commits into from
Jul 16, 2019
Merged

Conversation

mmmavis
Copy link
Collaborator

@mmmavis mmmavis commented Jul 10, 2019

Closes #3157

Test pages

61010126-e73c8300-a329-11e9-99d1-c3551a82a1c0

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

@patjouk patjouk temporarily deployed to foundation-mofostaging-pr-3383 July 10, 2019 22:24 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3383 July 10, 2019 22:46 Inactive
@mmmavis mmmavis requested a review from beccaklam July 10, 2019 23:05
Copy link

@beccaklam beccaklam left a 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.

Screen Shot 2019-07-11 at 10 39 36 AM

Tablet:

Buttons should fit in one line horizontally

Screen Shot 2019-07-11 at 10 38 08 AM

@mmmavis
Copy link
Collaborator Author

mmmavis commented Jul 11, 2019

@beccaklam

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.

@beccaklam
Copy link

@beccaklam

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? 😬

Screen Shot 2019-07-11 at 3 23 35 PM

@mmmavis
Copy link
Collaborator Author

mmmavis commented Jul 11, 2019

(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)
If the element is something designers can't turn into a Sketch Symbol
➡️ then it shouldn't be part of the styleguide as it means this element is something that can't be standardized
➡️ we should have design guidelines for this particular element for designers to follow when they create comps (e.g., make sure button icon is x pixels away from the button text, space in between two buttons is y pixels etc)

@beccaklam
Copy link

(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.

* #3321 (button level)

* #3322 (button group level)

* #3323 (both)

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)
If the element is something designers can't turn into a Sketch Symbol
➡️ then it shouldn't be part of the styleguide as it means this element is something that can't be standardized
➡️ we should have design guidelines for this particular element for designers to follow when they create comps (e.g., make sure button icon is x pixels away from the button text, space in between two buttons is y pixels etc)

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?

@beccaklam
Copy link

FWIW, this is what I'm thinking instead:

Desktop:

Screen Shot 2019-07-12 at 11 02 47 AM

Tablet

Screen Shot 2019-07-12 at 11 02 54 AM

Smaller Tablet

Post-MVP-tablet Copy 2@2x

Mobile

Screen Shot 2019-07-12 at 11 03 09 AM

But I do think we should talk it through with the larger design team first. Just dropping this here and I also dropped the designs into the blog redpen. And then we can decide how to update the tickets.

@beccaklam
Copy link

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:
Screen Shot 2019-07-15 at 4 06 42 PM
Let me know if you need anything else!

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3383 July 15, 2019 21:11 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3383 July 15, 2019 22:33 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3383 July 15, 2019 23:40 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3383 July 15, 2019 23:42 Inactive
@mmmavis
Copy link
Collaborator Author

mmmavis commented Jul 16, 2019

@beccaklam the minimum width for the Facebook button seems to be roughly 130px so I used that number instead of 113px. I also grouped the buttons in pairs so when the buttons get wrapped we won't have just one orphan button sitting on the second row for certain screen sizes. Let me know what you think.

image

Type
border 2px*2 = 4px
side padding 16px*2 = 32px
icon 16px = 16px
spacing between icon and text 8px = 8px
width of "Facebook" 70.35px = 70.35px
Total mininum width = 130.35px

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3383 July 16, 2019 00:04 Inactive
@mmmavis mmmavis requested a review from beccaklam July 16, 2019 00:05
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3383 July 16, 2019 00:05 Inactive
Copy link

@beccaklam beccaklam left a 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!

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3383 July 16, 2019 16:33 Inactive
@mmmavis mmmavis requested a review from Pomax July 16, 2019 16:58
@mmmavis
Copy link
Collaborator Author

mmmavis commented Jul 16, 2019

@Pomax this is ready for code review! 😃

@Pomax
Copy link
Contributor

Pomax commented Jul 16, 2019

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?

@mmmavis
Copy link
Collaborator Author

mmmavis commented Jul 16, 2019

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 😄

@mmmavis mmmavis merged commit 2a1b25b into master Jul 16, 2019
@mmmavis mmmavis deleted the issue-3157-blog-share-button-group branch July 23, 2019 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blog: add share buttons
4 participants