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

Foldable Card: Add border between summary and expand button if summary is included #2390

Merged
merged 4 commits into from Jan 19, 2016

Conversation

Projects
None yet
6 participants
@alternatekev
Contributor

alternatekev commented Jan 13, 2016

During the discussions of #1477 and #1898, it's become clear that we could use a divider between the expand/collapse icon and the action button (if present) on a FoldableCard. This icon helps divide the disclosure icon from a potentially destructive action, like accidentally disabling your Twitter connection.

Instead of forcing every single instance of FoldableCard that has an action button to inherit this style, I'm adding an iconBorder prop to the component, the rendering of which looks like this:

screen shot 2016-01-13 at 1 07 40 pm

screen shot 2016-01-13 at 1 07 47 pm

cc @mtias @kellychoffman @folletto

added new prop to FoldableCard allowing for iconBorder to be inserted…
… between open/close icon and action button to the left of it
Show outdated Hide outdated client/components/foldable-card/docs/example.jsx
@@ -55,6 +55,15 @@ module.exports = React.createClass( {
</p>
<p>
<FoldableCard
header="This is a FoldableCard with iconBorder"
iconBorder={ true }
summary={ <button className="button">Update</button> }

This comment has been minimized.

@mtias

mtias Jan 13, 2016

Member

Could we just render the border if there's content in summary?

@mtias

mtias Jan 13, 2016

Member

Could we just render the border if there's content in summary?

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 13, 2016

Contributor

@mtias

Could we just render the border if there's content in summary?

We could but this forces every FoldableCard to inherit this style, which I'm comfortable doing if others are ok with it.

Contributor

alternatekev commented Jan 13, 2016

@mtias

Could we just render the border if there's content in summary?

We could but this forces every FoldableCard to inherit this style, which I'm comfortable doing if others are ok with it.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Jan 13, 2016

Member

It would be every FoldableCard that has content to display on that right side, otherwise it remains with no border. I think it's a fair expectation and makes it a bit easier to use (instead of having to decide case by case). If there's content to the right, we render border, otherwise we don't.

Member

mtias commented Jan 13, 2016

It would be every FoldableCard that has content to display on that right side, otherwise it remains with no border. I think it's a fair expectation and makes it a bit easier to use (instead of having to decide case by case). If there's content to the right, we render border, otherwise we don't.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 13, 2016

Contributor

@mtias I went ahead and removed the iconBorder prop and instead based the has-border className on whether there's a summary prop. I can't find a ton of examples of it in use like this, but the one I could find visually was this, which seemed to work well:

screen shot 2016-01-13 at 3 28 22 pm

screen shot 2016-01-13 at 3 28 27 pm

Contributor

alternatekev commented Jan 13, 2016

@mtias I went ahead and removed the iconBorder prop and instead based the has-border className on whether there's a summary prop. I can't find a ton of examples of it in use like this, but the one I could find visually was this, which seemed to work well:

screen shot 2016-01-13 at 3 28 22 pm

screen shot 2016-01-13 at 3 28 27 pm

@alternatekev alternatekev changed the title from Foldable Card: Add iconBorder prop to Foldable Card: Add border between summary and expand button if summary is included Jan 13, 2016

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 13, 2016

Contributor

Also here, on all sites/single plugin view:

screen shot 2016-01-13 at 3 47 24 pm

screen shot 2016-01-13 at 3 47 16 pm

...although it doesn't seem super appropriate that it's showing up here?

Contributor

alternatekev commented Jan 13, 2016

Also here, on all sites/single plugin view:

screen shot 2016-01-13 at 3 47 24 pm

screen shot 2016-01-13 at 3 47 16 pm

...although it doesn't seem super appropriate that it's showing up here?

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Jan 13, 2016

Member

...although it doesn't seem super appropriate that it's showing up here?

Its only supposed to show when there's a button, right?

Member

kellychoffman commented Jan 13, 2016

...although it doesn't seem super appropriate that it's showing up here?

Its only supposed to show when there's a button, right?

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 13, 2016

Contributor

@kellychoffman It's now basing the line on whether the summary prop is used, as suggested by @mtias, but the summary is being used here to pass a PluginUpdateIndicator on this line:

https://github.com/Automattic/wp-calypso/blob/master/client/my-sites/plugins/plugin-site-jetpack/index.jsx#L57

Contributor

alternatekev commented Jan 13, 2016

@kellychoffman It's now basing the line on whether the summary prop is used, as suggested by @mtias, but the summary is being used here to pass a PluginUpdateIndicator on this line:

https://github.com/Automattic/wp-calypso/blob/master/client/my-sites/plugins/plugin-site-jetpack/index.jsx#L57

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Jan 13, 2016

Member

Okay. I don't think its bad, just doesn't seem needed.

Member

kellychoffman commented Jan 13, 2016

Okay. I don't think its bad, just doesn't seem needed.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 13, 2016

Contributor

Agreed. We can probably update that (little-seen) plugins component to not use the summary prop?

Contributor

alternatekev commented Jan 13, 2016

Agreed. We can probably update that (little-seen) plugins component to not use the summary prop?

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 13, 2016

Contributor

This is what it looks like with the Updater component showing:

untitled-1
untitled-2

we can refactor it after the FoldableCard is updated, and have it only pass in the summary prop when there's actually an update, instead of using it every time, update or no. (and obviously, the top one would lose the border in that process) (and we can fix the margin on the button which is weird and unrelated)

Contributor

alternatekev commented Jan 13, 2016

This is what it looks like with the Updater component showing:

untitled-1
untitled-2

we can refactor it after the FoldableCard is updated, and have it only pass in the summary prop when there's actually an update, instead of using it every time, update or no. (and obviously, the top one would lose the border in that process) (and we can fix the margin on the button which is weird and unrelated)

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Jan 13, 2016

Member

Makes sense.

Member

kellychoffman commented Jan 13, 2016

Makes sense.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 14, 2016

Contributor

Once everything is merged and a tiny css update is made, it'll look like this on sharing @kellychoffman

screen shot 2016-01-14 at 2 43 03 pm

Contributor

alternatekev commented Jan 14, 2016

Once everything is merged and a tiny css update is made, it'll look like this on sharing @kellychoffman

screen shot 2016-01-14 at 2 43 03 pm

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Jan 15, 2016

Member

Once everything is merged and a tiny css update is made, it'll look like this on sharing @kellychoffman

I dig it.

Member

kellychoffman commented Jan 15, 2016

Once everything is merged and a tiny css update is made, it'll look like this on sharing @kellychoffman

I dig it.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 15, 2016

Contributor

Pinging @dmsnell for code review

Contributor

alternatekev commented Jan 15, 2016

Pinging @dmsnell for code review

Show outdated Hide outdated client/components/foldable-card/index.jsx
@@ -41,7 +41,7 @@ var FoldableCard = React.createClass( {
onClose: noop,
cardKey: '',
icon: 'chevron-down',
isExpanded: false
isExpanded: false,

This comment has been minimized.

@dmsnell

dmsnell Jan 15, 2016

Contributor

is this a typo?

@dmsnell

dmsnell Jan 15, 2016

Contributor

is this a typo?

This comment has been minimized.

@folletto

folletto Jan 18, 2016

Member

I thought it was good practice to end all lines in objects with a comma to avoid double-line diffs when a single line gets added (clarity) and making all the lines uniform.

@folletto

folletto Jan 18, 2016

Member

I thought it was good practice to end all lines in objects with a comma to avoid double-line diffs when a single line gets added (clarity) and making all the lines uniform.

This comment has been minimized.

@dmsnell

dmsnell Jan 18, 2016

Contributor

I couldn't find this explicit in our style guide. There was one example that included a list and it had the trailing comma. Most of the time I see them I feel like the don't have the trailing comma, but Babel handles it gracefully, so maybe this boils down to personal preference here.

@dmsnell

dmsnell Jan 18, 2016

Contributor

I couldn't find this explicit in our style guide. There was one example that included a list and it had the trailing comma. Most of the time I see them I feel like the don't have the trailing comma, but Babel handles it gracefully, so maybe this boils down to personal preference here.

This comment has been minimized.

@alternatekev

alternatekev Jan 18, 2016

Contributor

Typo, since I had an extra prop defined at first, which I removed. I'll remove the comma here.

@alternatekev

alternatekev Jan 18, 2016

Contributor

Typo, since I had an extra prop defined at first, which I removed. I'll remove the comma here.

This comment has been minimized.

@alternatekev

alternatekev Jan 18, 2016

Contributor

Fixed.

@alternatekev

alternatekev Jan 18, 2016

Contributor

Fixed.

@dmsnell

This comment has been minimized.

Show comment
Hide comment
@dmsnell

dmsnell Jan 15, 2016

Contributor

@alternatekev I don't know exactly what summary means, but I like the way of automatically setting this attribute instead of relying on a prop. I'm not against these kinds of boolean props, but I think that if it's true in 95%+ of the cases we use in Calypso, then just making that the default behavior is great.

Contributor

dmsnell commented Jan 15, 2016

@alternatekev I don't know exactly what summary means, but I like the way of automatically setting this attribute instead of relying on a prop. I'm not against these kinds of boolean props, but I think that if it's true in 95%+ of the cases we use in Calypso, then just making that the default behavior is great.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 18, 2016

Contributor

Adding screenshots of more screens that will update with this merge:

Exporter:
screen shot 2016-01-18 at 3 25 22 pm

Contributor

alternatekev commented Jan 18, 2016

Adding screenshots of more screens that will update with this merge:

Exporter:
screen shot 2016-01-18 at 3 25 22 pm

@dmsnell

This comment has been minimized.

Show comment
Hide comment
@dmsnell

dmsnell Jan 18, 2016

Contributor

:shipit:

Contributor

dmsnell commented Jan 18, 2016

:shipit:

alternatekev added a commit that referenced this pull request Jan 19, 2016

Merge pull request #2390 from Automattic/add/foldable-card-icon-border
Foldable Card: Add border between summary and expand button if summary is included

@alternatekev alternatekev merged commit ae62261 into master Jan 19, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@alternatekev alternatekev deleted the add/foldable-card-icon-border branch Jan 19, 2016

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