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

Sharing: Update to FoldableCard Pattern #1474

Merged
merged 8 commits into from Feb 2, 2016

Conversation

Projects
None yet
@alternatekev
Contributor

alternatekev commented Dec 10, 2015

This PR updates Sharing to the FoldableCard components. Currently in production, Sharing looks like this:
screen shot 2016-01-14 at 3 03 17 pm

When updating to FoldableCards, this PR looks like this:

screen shot 2016-01-26 at 4 35 29 pm

This also makes use of the new SocialLogo component introduced by @enejb. Since Twitter doesn't have a box-surrounded version of the icon, I went with mostly non-boxed icons here. We can easily change this to include boxed icons for every logo, but it'd require adding one for Twitter to the SocialLogo component.

Pinging @johnHackworth, @rickybanister and @mtias for reviews.

@alternatekev alternatekev changed the title from Sharing: Update Connect buttons to borderless Button to Sharing: Update Disconnect buttons to borderless Button Dec 10, 2015

@rickybanister

This comment has been minimized.

Show comment
Hide comment
@rickybanister

rickybanister Dec 10, 2015

Member

The borderless buttons look just fine to me.

Do you think it would be possible (later on, different PR) to switch to foldable card for these?

Member

rickybanister commented Dec 10, 2015

The borderless buttons look just fine to me.

Do you think it would be possible (later on, different PR) to switch to foldable card for these?

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Dec 10, 2015

Contributor

@rickybanister Yep, that was going to be my next step. Do we even need them foldable if there's no connection? The preview screenshots don't seem to add much.

Contributor

alternatekev commented Dec 10, 2015

@rickybanister Yep, that was going to be my next step. Do we even need them foldable if there's no connection? The preview screenshots don't seem to add much.

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Dec 11, 2015

Member

What is the reasoning behind the change?

Member

kellychoffman commented Dec 11, 2015

What is the reasoning behind the change?

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Dec 11, 2015

Member

switch to foldable card

Do we even need them foldable if there's no connection? The preview screenshots don't seem to add much.

We are planning on updating the page to use more components, such as the foldable card. The preview screenshots are important in that they tell a new user what they can get out of connecting without sending them to a support doc.

Member

kellychoffman commented Dec 11, 2015

switch to foldable card

Do we even need them foldable if there's no connection? The preview screenshots don't seem to add much.

We are planning on updating the page to use more components, such as the foldable card. The preview screenshots are important in that they tell a new user what they can get out of connecting without sending them to a support doc.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Dec 11, 2015

Contributor

@kellychoffman Updated to remove the icon:

screen shot 2015-12-11 at 4 04 40 pm

Contributor

alternatekev commented Dec 11, 2015

@kellychoffman Updated to remove the icon:

screen shot 2015-12-11 at 4 04 40 pm

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Dec 13, 2015

Member

It looks a bit off when mixed with services that aren't connected. Maybe because it isn't aligned.
screen shot 2015-12-13 at 10 06 27 am

Do you think Disconnect looks like a link?

Member

kellychoffman commented Dec 13, 2015

It looks a bit off when mixed with services that aren't connected. Maybe because it isn't aligned.
screen shot 2015-12-13 at 10 06 27 am

Do you think Disconnect looks like a link?

@rickybanister

This comment has been minimized.

Show comment
Hide comment
@rickybanister

rickybanister Dec 16, 2015

Member

@kellychoffman would it be inappropriate for it to be red? Is disconnecting a sharing service a 'scary' thing? It would then be inline with many other 'remove' and 'delete' items.

Member

rickybanister commented Dec 16, 2015

@kellychoffman would it be inappropriate for it to be red? Is disconnecting a sharing service a 'scary' thing? It would then be inline with many other 'remove' and 'delete' items.

Show outdated Hide outdated client/my-sites/sharing/connections/service-action.jsx
@@ -1,12 +1,15 @@
/**
* External dependencies
*/
var React = require( 'react' );
import React from "react";

This comment has been minimized.

@gwwar

gwwar Dec 16, 2015

Member

Single quotes please. :)

@gwwar

gwwar Dec 16, 2015

Member

Single quotes please. :)

Show outdated Hide outdated client/my-sites/sharing/connections/service-action.jsx
import serviceConnections from "./service-connections";
import Gridicon from "components/gridicon";
import Button from "components/button";
import classNames from "classnames";

This comment has been minimized.

@gwwar

gwwar Dec 16, 2015

Member

Single quotes please. :)

@gwwar

gwwar Dec 16, 2015

Member

Single quotes please. :)

Show outdated Hide outdated client/my-sites/sharing/connections/service-action.jsx
@@ -38,27 +41,32 @@ module.exports = React.createClass( {
},
render: function() {
var classes = [ 'sharing-service-action', 'button' ],
isPending, removableConnections, label;
var classes = [ 'sharing-service-action' ],

This comment has been minimized.

@gwwar

gwwar Dec 16, 2015

Member

We can use let here instead of var

@gwwar

gwwar Dec 16, 2015

Member

We can use let here instead of var

@gwwar

This comment has been minimized.

Show comment
Hide comment
@gwwar

gwwar Dec 16, 2015

Member

Besides some minor changes the code looks good 👍 This branch needs to rebased.

Do you think Disconnect looks like a link?

It does. Having this large of a click target for something that looks like a link, is a bit odd to me.

clickable

Member

gwwar commented Dec 16, 2015

Besides some minor changes the code looks good 👍 This branch needs to rebased.

Do you think Disconnect looks like a link?

It does. Having this large of a click target for something that looks like a link, is a bit odd to me.

clickable

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Dec 16, 2015

Contributor

So.. I sort of revamped the whole thing and came up with this, which should solve most people's issues with this page for a while. We could probably get rid of the icons though:

screen shot 2015-12-16 at 2 30 04 pm

Contributor

alternatekev commented Dec 16, 2015

So.. I sort of revamped the whole thing and came up with this, which should solve most people's issues with this page for a while. We could probably get rid of the icons though:

screen shot 2015-12-16 at 2 30 04 pm

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Dec 16, 2015

Contributor

Here's how it looks icon-less:

screen shot 2015-12-16 at 2 42 07 pm

Contributor

alternatekev commented Dec 16, 2015

Here's how it looks icon-less:

screen shot 2015-12-16 at 2 42 07 pm

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Dec 16, 2015

Contributor

The above changes would work well also on #1477

Contributor

alternatekev commented Dec 16, 2015

The above changes would work well also on #1477

@rickybanister

This comment has been minimized.

Show comment
Hide comment
@rickybanister

rickybanister Dec 16, 2015

Member

This isn't my game, but icon-less is better. Ironically we're nearly back to what it was before. Just with the new foldable card style.

Member

rickybanister commented Dec 16, 2015

This isn't my game, but icon-less is better. Ironically we're nearly back to what it was before. Just with the new foldable card style.

@rralian

This comment has been minimized.

Show comment
Hide comment
@rralian

rralian Dec 16, 2015

Contributor

@alternatekev is the mock here just a visual mockup or was it a screenshot? The alignment of the icons is inconsistent. Otherwise I don't have a dog in the icon/iconless fight.

Contributor

rralian commented Dec 16, 2015

@alternatekev is the mock here just a visual mockup or was it a screenshot? The alignment of the icons is inconsistent. Otherwise I don't have a dog in the icon/iconless fight.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Dec 17, 2015

Member

I like the compact buttons since they weight less in a page otherwise full of buttons.

Member

mtias commented Dec 17, 2015

I like the compact buttons since they weight less in a page otherwise full of buttons.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Dec 17, 2015

Contributor

@rralian This was a screenshot, and you're right about the icon alignment. It was looking super weird to me too, which should probably be its own PR/issue.

Contributor

alternatekev commented Dec 17, 2015

@rralian This was a screenshot, and you're right about the icon alignment. It was looking super weird to me too, which should probably be its own PR/issue.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Dec 17, 2015

Contributor

This isn't my game, but icon-less is better. Ironically we're nearly back to what it was before. Just with the new foldable card style.

Yeah, I got myself to a place where I didn't think the borderless was working here in practice. To be fair, we're using the compact buttons which I think make a ton of difference, especially when taking into account the foldable card updates. It feels correct to me, which isn't to say it 100% is.

Contributor

alternatekev commented Dec 17, 2015

This isn't my game, but icon-less is better. Ironically we're nearly back to what it was before. Just with the new foldable card style.

Yeah, I got myself to a place where I didn't think the borderless was working here in practice. To be fair, we're using the compact buttons which I think make a ton of difference, especially when taking into account the foldable card updates. It feels correct to me, which isn't to say it 100% is.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Dec 17, 2015

Contributor

@kellychoffman what do you think about moving eventbrite up to the main area and treating it the same? does it need to be in its own group down there:

screen shot 2015-12-17 at 11 24 07 am

Contributor

alternatekev commented Dec 17, 2015

@kellychoffman what do you think about moving eventbrite up to the main area and treating it the same? does it need to be in its own group down there:

screen shot 2015-12-17 at 11 24 07 am

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Dec 17, 2015

Member

I was out for the past two days... Catching up.

icon-less is better. Ironically we're nearly back to what it was before.

I like that we are back to the buttons, but I'm not convinced compact is better than the original buttons. For a new user, Connecting is the primary (only) action for this page. Also, I agree about the icons - I'm not sure they are adding anything here.

what do you think about moving eventbrite up to the main area and treating it the same? does it need to be in its own group down there:

There's going to be other services down there. Also Eventbrite isn't publicizing, so I'd rather not group it up with those services.

Member

kellychoffman commented Dec 17, 2015

I was out for the past two days... Catching up.

icon-less is better. Ironically we're nearly back to what it was before.

I like that we are back to the buttons, but I'm not convinced compact is better than the original buttons. For a new user, Connecting is the primary (only) action for this page. Also, I agree about the icons - I'm not sure they are adding anything here.

what do you think about moving eventbrite up to the main area and treating it the same? does it need to be in its own group down there:

There's going to be other services down there. Also Eventbrite isn't publicizing, so I'd rather not group it up with those services.

@jkudish

This comment has been minimized.

Show comment
Hide comment
@jkudish

jkudish Dec 17, 2015

Contributor

I agree with @kellychoffman -- grouping the services would be pretty confusing for users. There will eventually be other services there too... What Eventbrite does, and how it works is very different from Publicize. I've even advocated for a different tab for it in the past, though I understand why we didn't do that (at least for now).

Contributor

jkudish commented Dec 17, 2015

I agree with @kellychoffman -- grouping the services would be pretty confusing for users. There will eventually be other services there too... What Eventbrite does, and how it works is very different from Publicize. I've even advocated for a different tab for it in the past, though I understand why we didn't do that (at least for now).

@alternatekev alternatekev changed the title from Sharing: Update Disconnect buttons to borderless Button to Sharing: Update to FoldableCard Pattern Dec 18, 2015

@folletto

This comment has been minimized.

Show comment
Hide comment
@folletto

folletto Jan 8, 2016

Member

I give it a run on Chrome, Firefox, Safari — feels it's working really well to me. :)

One minor thing I noticed, is that the animation on the chevron has disappeared. But I guess it's because this is using the same FoldableCard pattern, but not the FoldableCard component, right?

Member

folletto commented Jan 8, 2016

I give it a run on Chrome, Firefox, Safari — feels it's working really well to me. :)

One minor thing I noticed, is that the animation on the chevron has disappeared. But I guess it's because this is using the same FoldableCard pattern, but not the FoldableCard component, right?

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 8, 2016

Contributor

The version of this PR that you can test is using faked FoldableCards, yse. I’ve got commits waiting to be pushed that replace the whole thing with actual FoldableCards.

On Jan 8, 2016, at 6:08 AM, Davide Casali notifications@github.com wrote:

Contributor

alternatekev commented Jan 8, 2016

The version of this PR that you can test is using faked FoldableCards, yse. I’ve got commits waiting to be pushed that replace the whole thing with actual FoldableCards.

On Jan 8, 2016, at 6:08 AM, Davide Casali notifications@github.com wrote:

@folletto

This comment has been minimized.

Show comment
Hide comment
@folletto

folletto Jan 8, 2016

Member

Ok, cool! :)

Member

folletto commented Jan 8, 2016

Ok, cool! :)

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 14, 2016

Contributor

This one's ready for review @folletto @kellychoffman @mtias.

It doesn't include the border since that's a part of the FoldableCard update which you can see a preview of here: #2390 (comment)

Contributor

alternatekev commented Jan 14, 2016

This one's ready for review @folletto @kellychoffman @mtias.

It doesn't include the border since that's a part of the FoldableCard update which you can see a preview of here: #2390 (comment)

@rickybanister

This comment has been minimized.

Show comment
Hide comment
@rickybanister

rickybanister Jan 14, 2016

Member

What does this look like now? Could update the initial description a bit since things have changed quite a lot in a month?

Member

rickybanister commented Jan 14, 2016

What does this look like now? Could update the initial description a bit since things have changed quite a lot in a month?

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 14, 2016

Contributor

@rickybanister yep, updating now.

Contributor

alternatekev commented Jan 14, 2016

@rickybanister yep, updating now.

@drw158

This comment has been minimized.

Show comment
Hide comment
@drw158

drw158 Jan 14, 2016

Member

If using the social icons feels misaligned, please note it in the social icons repo so we can get it fixed.

Member

drw158 commented Jan 14, 2016

If using the social icons feels misaligned, please note it in the social icons repo so we can get it fixed.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 27, 2016

Contributor

Pinging @enejb for another review.

Contributor

alternatekev commented Jan 27, 2016

Pinging @enejb for another review.

@enejb

This comment has been minimized.

Show comment
Hide comment
@enejb

enejb Jan 27, 2016

Member

I am still able to see ^ error.
screen shot 2016-01-26 at 16 49 41

Thanks for fixing the other note that I left there.

Member

enejb commented Jan 27, 2016

I am still able to see ^ error.
screen shot 2016-01-26 at 16 49 41

Thanks for fixing the other note that I left there.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 27, 2016

Contributor

@enejb that's weird, it should be looking like this now:

screen shot 2016-01-26 at 7 58 34 pm

screen shot 2016-01-26 at 7 59 37 pm

Contributor

alternatekev commented Jan 27, 2016

@enejb that's weird, it should be looking like this now:

screen shot 2016-01-26 at 7 58 34 pm

screen shot 2016-01-26 at 7 59 37 pm

@enejb

This comment has been minimized.

Show comment
Hide comment
@enejb

enejb Jan 27, 2016

Member

Maybe I am testing a width that is too narrow?
Will try to make sure that my CSS is also the latest version just in case.

On Tuesday, 26 January 2016, Kevin Conboy notifications@github.com wrote:

@enejb https://github.com/enejb that's weird, it should be looking like
this now:

[image: screen shot 2016-01-26 at 7 58 34 pm]
https://cloud.githubusercontent.com/assets/1084656/12602527/3e4f6b80-c467-11e5-9d45-13a77eff4fc3.png


Reply to this email directly or view it on GitHub
#1474 (comment)
.

Member

enejb commented Jan 27, 2016

Maybe I am testing a width that is too narrow?
Will try to make sure that my CSS is also the latest version just in case.

On Tuesday, 26 January 2016, Kevin Conboy notifications@github.com wrote:

@enejb https://github.com/enejb that's weird, it should be looking like
this now:

[image: screen shot 2016-01-26 at 7 58 34 pm]
https://cloud.githubusercontent.com/assets/1084656/12602527/3e4f6b80-c467-11e5-9d45-13a77eff4fc3.png


Reply to this email directly or view it on GitHub
#1474 (comment)
.

@folletto

This comment has been minimized.

Show comment
Hide comment
@folletto

folletto Jan 27, 2016

Member

I get the same issue in Chrome and Safari (iPhone 5 emulation):

screen shot 2016-01-27 at 10 57 07

Does not happen in Firefox.

Starts happening below ~350px of width.

Maybe we should refactor that part of the component to use flex which will make the component far more solid?

Member

folletto commented Jan 27, 2016

I get the same issue in Chrome and Safari (iPhone 5 emulation):

screen shot 2016-01-27 at 10 57 07

Does not happen in Firefox.

Starts happening below ~350px of width.

Maybe we should refactor that part of the component to use flex which will make the component far more solid?

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 27, 2016

Contributor

@folletto

Maybe we should refactor that part of the component to use flex which will make the component far more solid?

I'd be happy to do this. Sharing & Connected apps both use custom CSS to do their icon/layout work which get in the way of the component's native rendering a bit. Flex box would help out the FoldableCard header and summary props considerably.

In fact, it looks like we use it already. By playing around with it in the browser, I found a better flexbox layout that fixes the issue.

Contributor

alternatekev commented Jan 27, 2016

@folletto

Maybe we should refactor that part of the component to use flex which will make the component far more solid?

I'd be happy to do this. Sharing & Connected apps both use custom CSS to do their icon/layout work which get in the way of the component's native rendering a bit. Flex box would help out the FoldableCard header and summary props considerably.

In fact, it looks like we use it already. By playing around with it in the browser, I found a better flexbox layout that fixes the issue.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 27, 2016

Contributor

@folletto @enejb I addressed the flexbox layout in #2835

Contributor

alternatekev commented Jan 27, 2016

@folletto @enejb I addressed the flexbox layout in #2835

@enejb

This comment has been minimized.

Show comment
Hide comment
@enejb

enejb Jan 28, 2016

Member

There are still some widths that looks strange to me when I do the test the page and resize it.
For example
screen shot 2016-01-27 at 15 54 33

Which to me just looks odd/broken.

Member

enejb commented Jan 28, 2016

There are still some widths that looks strange to me when I do the test the page and resize it.
For example
screen shot 2016-01-27 at 15 54 33

Which to me just looks odd/broken.

@folletto

This comment has been minimized.

Show comment
Hide comment
@folletto

folletto Jan 28, 2016

Member

It's a bit odd, but better than live right now where the text at certain breakpoints goes below the button. :P

We can probably fix the content with some extra rule (again flex?) to make that more flexible as well.

Member

folletto commented Jan 28, 2016

It's a bit odd, but better than live right now where the text at certain breakpoints goes below the button. :P

We can probably fix the content with some extra rule (again flex?) to make that more flexible as well.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 29, 2016

Contributor

I'm going to do a new PR to adjust the flex widths at small sizes so I think this one is ready to merge?

Contributor

alternatekev commented Jan 29, 2016

I'm going to do a new PR to adjust the flex widths at small sizes so I think this one is ready to merge?

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Jan 29, 2016

Member

I think we should keep the white icons with the colored background. The updated colored icons looks too messy.

Member

kellychoffman commented Jan 29, 2016

I think we should keep the white icons with the colored background. The updated colored icons looks too messy.

@MichaelArestad

This comment has been minimized.

Show comment
Hide comment
@MichaelArestad

MichaelArestad Jan 29, 2016

Contributor

Just out of curiosity, why were the icons taken out of the colored backgrounds? It looked more uniform before.

Contributor

MichaelArestad commented Jan 29, 2016

Just out of curiosity, why were the icons taken out of the colored backgrounds? It looked more uniform before.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 29, 2016

Contributor

The icons were changed because there's a new SocialLogo component that includes all of the icons within a roundrect except for Twitter. We could go with all roundrect backgrounds but I need one for Twitter added to the component.

Contributor

alternatekev commented Jan 29, 2016

The icons were changed because there's a new SocialLogo component that includes all of the icons within a roundrect except for Twitter. We could go with all roundrect backgrounds but I need one for Twitter added to the component.

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Jan 29, 2016

Member

Could we use the non roundrect versions of all the logos instead? (I think Facebook is the only one without one) For Facebook, we could use the roundrect version and just make sure it matches the border radius of the others.

Member

kellychoffman commented Jan 29, 2016

Could we use the non roundrect versions of all the logos instead? (I think Facebook is the only one without one) For Facebook, we could use the roundrect version and just make sure it matches the border radius of the others.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 29, 2016

Contributor

I really wanted to avoid doing custom CSS for all of the logos except one. If almost all have roundrect versions, it shouldn't be too hard to add just one. Also it makes the alignment of the logo easier because it's being done in SVG vs aligning it around in CSS.

Contributor

alternatekev commented Jan 29, 2016

I really wanted to avoid doing custom CSS for all of the logos except one. If almost all have roundrect versions, it shouldn't be too hard to add just one. Also it makes the alignment of the logo easier because it's being done in SVG vs aligning it around in CSS.

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Jan 29, 2016

Member

Fair. I think we should avoid making updates to the social icons until we have either all roundrect or all standalone.

Member

kellychoffman commented Jan 29, 2016

Fair. I think we should avoid making updates to the social icons until we have either all roundrect or all standalone.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 29, 2016

Contributor

By adding a roundrect version of Twitter (which I'm just about done with), then this page will have all roundrect. Except Google+ which will be a circle and Eventbrite which will have its off-kilter rectangle. Would that work?

Contributor

alternatekev commented Jan 29, 2016

By adding a roundrect version of Twitter (which I'm just about done with), then this page will have all roundrect. Except Google+ which will be a circle and Eventbrite which will have its off-kilter rectangle. Would that work?

@drw158

This comment has been minimized.

Show comment
Hide comment
@drw158

drw158 Jan 29, 2016

Member

By adding a roundrect version of Twitter (which I'm just about done with), then this page will have all roundrect. Except Google+ which will be a circle and Eventbrite which will have its off-kilter rectangle. Would that work?

That works for me. I'm not opposed to including a version of the Twitter logo with a rectangle in Social Logos.

Member

drw158 commented Jan 29, 2016

By adding a roundrect version of Twitter (which I'm just about done with), then this page will have all roundrect. Except Google+ which will be a circle and Eventbrite which will have its off-kilter rectangle. Would that work?

That works for me. I'm not opposed to including a version of the Twitter logo with a rectangle in Social Logos.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 30, 2016

Contributor

Could we use the non roundrect versions of all the logos instead? (I think Facebook is the only one without one) For Facebook, we could use the roundrect version and just make sure it matches the border radius of the others.

@kellychoffman I totally misread this as you wanting to use ALL roundrects. This is currently what this PR does, except for LinkedIn, which also is only in a roundrect format.

Using all non-roundrect except for LinkedIn and Facebook is what this PR does now.

I've already got the SocialLogos component updated with a pending PR to add the Twitter roundrect... we could see how that version stacks up against what's in the screenshot at the top and go from there.

I always forget about Eventbrite I guess. It seems rounrect-enough to me?

Contributor

alternatekev commented Jan 30, 2016

Could we use the non roundrect versions of all the logos instead? (I think Facebook is the only one without one) For Facebook, we could use the roundrect version and just make sure it matches the border radius of the others.

@kellychoffman I totally misread this as you wanting to use ALL roundrects. This is currently what this PR does, except for LinkedIn, which also is only in a roundrect format.

Using all non-roundrect except for LinkedIn and Facebook is what this PR does now.

I've already got the SocialLogos component updated with a pending PR to add the Twitter roundrect... we could see how that version stacks up against what's in the screenshot at the top and go from there.

I always forget about Eventbrite I guess. It seems rounrect-enough to me?

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Feb 1, 2016

Member

Using all non-roundrect except for LinkedIn and Facebook is what this PR does now.

Right, but not with colored backgrounds as it was before.

I always forget about Eventbrite I guess. It seems rounrect-enough to me?

Yeah I think its fine on its own.

Member

kellychoffman commented Feb 1, 2016

Using all non-roundrect except for LinkedIn and Facebook is what this PR does now.

Right, but not with colored backgrounds as it was before.

I always forget about Eventbrite I guess. It seems rounrect-enough to me?

Yeah I think its fine on its own.

alternatekev added some commits Dec 10, 2015

replacing RemoveButton with Button borderless
moved sharing.scss to my-sites/sharing/style and updated imports

removing icon

made buttons small and added icons

cleaned up page w/r/t foldable-card pattern

fixed padding for proper hover fx on cards

removed the icons again

replacing RemoveButton with Button borderless

moved sharing.scss to my-sites/sharing/style and updated imports

removing icon

made buttons small and added icons

fixed padding for proper hover fx on cards

removed the icons again
working foldablecard into sharing connections
working foldablecard into sharing connections

working foldablecard into sharing connections

working foldablecard into sharing connections

removed extraneous service-logo component i'd started
updated placeholder/loaders
removed extraneous service logo svgs

working foldablecard into sharing connections

working foldablecard into sharing connections

working foldablecard into sharing connections

working foldablecard into sharing connections

removed extraneous service-logo component i'd started

css updates to clarify interface + removal of subheader

updated placeholder/loaders

removed extraneous service logo svgs

slides action button to the left 8px in response to the new foldable card that's been merged

removed conflicting CSS classes

formatting fixes
fixed mobile issues, mostly by removing CSS and using compact prop on…
… foldable-card

fixed service names on small mobile sizes
improved placeholders by using same components as main layout which h…
…elped mobile issues

fixed linting errors

replacing RemoveButton with Button borderless

moved sharing.scss to my-sites/sharing/style and updated imports

removing icon

made buttons small and added icons

cleaned up page w/r/t foldable-card pattern

fixed padding for proper hover fx on cards

removed the icons again

replacing RemoveButton with Button borderless

moved sharing.scss to my-sites/sharing/style and updated imports

removing icon

made buttons small and added icons

fixed padding for proper hover fx on cards

removed the icons again

working foldablecard into sharing connections

working foldablecard into sharing connections

working foldablecard into sharing connections

working foldablecard into sharing connections

removed extraneous service-logo component i'd started

css updates to clarify interface + removal of subheader

updated placeholder/loaders

removed extraneous service logo svgs

working foldablecard into sharing connections

working foldablecard into sharing connections

working foldablecard into sharing connections

working foldablecard into sharing connections

removed extraneous service-logo component i'd started

css updates to clarify interface + removal of subheader

updated placeholder/loaders

removed extraneous service logo svgs

slides action button to the left 8px in response to the new foldable card that's been merged

fixed mobile issues, mostly by removing CSS and using compact prop on foldable-card

fixed service names on small mobile sizes

fixed linting errors
updated logos to roundrect versions, google+ is a circle and eventbri…
…te retains its off-filter container

replacing RemoveButton with Button borderless

moved sharing.scss to my-sites/sharing/style and updated imports

removing icon

made buttons small and added icons

cleaned up page w/r/t foldable-card pattern

fixed padding for proper hover fx on cards

removed the icons again

replacing RemoveButton with Button borderless

moved sharing.scss to my-sites/sharing/style and updated imports

removing icon

made buttons small and added icons

fixed padding for proper hover fx on cards

removed the icons again

working foldablecard into sharing connections

working foldablecard into sharing connections

working foldablecard into sharing connections

working foldablecard into sharing connections

removed extraneous service-logo component i'd started

css updates to clarify interface + removal of subheader

removed extraneous service logo svgs

working foldablecard into sharing connections

working foldablecard into sharing connections

working foldablecard into sharing connections

working foldablecard into sharing connections

removed extraneous service-logo component i'd started

css updates to clarify interface + removal of subheader

updated placeholder/loaders

removed extraneous service logo svgs

slides action button to the left 8px in response to the new foldable card that's been merged

formatting fixes

fixed mobile issues, mostly by removing CSS and using compact prop on foldable-card

fixed service names on small mobile sizes

fixed linting errors

replacing RemoveButton with Button borderless

moved sharing.scss to my-sites/sharing/style and updated imports

removing icon

made buttons small and added icons

cleaned up page w/r/t foldable-card pattern

fixed padding for proper hover fx on cards

removed the icons again

replacing RemoveButton with Button borderless

moved sharing.scss to my-sites/sharing/style and updated imports

removing icon

made buttons small and added icons

fixed padding for proper hover fx on cards

removed the icons again

working foldablecard into sharing connections

working foldablecard into sharing connections

working foldablecard into sharing connections

working foldablecard into sharing connections

removed extraneous service-logo component i'd started

css updates to clarify interface + removal of subheader

updated placeholder/loaders

removed extraneous service logo svgs

working foldablecard into sharing connections

working foldablecard into sharing connections

working foldablecard into sharing connections

working foldablecard into sharing connections

removed extraneous service-logo component i'd started

css updates to clarify interface + removal of subheader

updated placeholder/loaders

removed extraneous service logo svgs

slides action button to the left 8px in response to the new foldable card that's been merged

fixed mobile issues, mostly by removing CSS and using compact prop on foldable-card

fixed service names on small mobile sizes

fixed linting errors

updated logos to roundrect versions, google+ is a circle and eventbrite retains its off-filter container

alternatekev added a commit that referenced this pull request Feb 2, 2016

Merge pull request #1474 from Automattic/update/sharing-buttons
Sharing: Update to FoldableCard Pattern

@alternatekev alternatekev merged commit 7b3ea56 into master Feb 2, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@enejb enejb deleted the update/sharing-buttons branch Feb 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment