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

Connected Applications: Update to FoldableCard Pattern #1477

Merged
merged 6 commits into from Jan 27, 2016

Conversation

Projects
None yet
7 participants
@alternatekev
Contributor

alternatekev commented Dec 10, 2015

As part of my attempt to replace custom foldable-card style components with the standard global FoldableCard component, this PR takes the Connected Applications screen, which looks like this and is built with a custom set of set of components and SCSS...
screen shot 2016-01-25 at 4 24 43 pm

...and turns it into this, which uses the standard FoldableCard component:
screen shot 2016-01-26 at 10 02 27 am

This PR brings Connected Applications into parity with the new Sharing page, PR #1474.

To test: check out the branch and head on over to /me/security/connected-applications and you should see the updated layout.

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Dec 10, 2015

Member

I'm not sure it should have the 'remove link' icon there. The "Disconnect" text on its own says it all.

Member

kellychoffman commented Dec 10, 2015

I'm not sure it should have the 'remove link' icon there. The "Disconnect" text on its own says it all.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Dec 10, 2015

Contributor

@kellychoffman Do you think that's true of the screenshots in #1214 as well? I'd rather these two instances share the same treatment, although one is scary and one isn't.

Contributor

alternatekev commented Dec 10, 2015

@kellychoffman Do you think that's true of the screenshots in #1214 as well? I'd rather these two instances share the same treatment, although one is scary and one isn't.

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Dec 11, 2015

Member

I missed the sharing PR. I'm not sure its necessary there. Why change it?

Member

kellychoffman commented Dec 11, 2015

I missed the sharing PR. I'm not sure its necessary there. Why change it?

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Dec 11, 2015

Contributor

@kellychoffman The goal is just to keep them in sync because they say and more or less do the same thing. The difference in their "undo-ability" is indicated by the red color of the button in this PR. My goal is for anything that says "Disconnect" to work/look the same way. If I changed it here per your recommendation I'd be changing the Disconnect pattern.

Also, these two screens both use a wonky faux FoldableCard pattern that should also be synced up in a future PR, so I'd rather keep everything about these screens working/looking the same.

Contributor

alternatekev commented Dec 11, 2015

@kellychoffman The goal is just to keep them in sync because they say and more or less do the same thing. The difference in their "undo-ability" is indicated by the red color of the button in this PR. My goal is for anything that says "Disconnect" to work/look the same way. If I changed it here per your recommendation I'd be changing the Disconnect pattern.

Also, these two screens both use a wonky faux FoldableCard pattern that should also be synced up in a future PR, so I'd rather keep everything about these screens working/looking the same.

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Dec 11, 2015

Member

My goal is for anything that says "Disconnect" to work/look the same way

I think there can be different degrees of severity. Disconnect that could break something and disconnect that will turn something off. Sharing is an example of the latter. You disconnect Facebook, and nothing breaks, it just prevents you from Publicizing to Facebook automatically in the future. I don't think this warrants a scary style. What I could see happening here, however is a confirmation dialog.

Also, these two screens both use a wonky faux FoldableCard pattern that should also be synced up in a future PR

👍 Totally agree.

Member

kellychoffman commented Dec 11, 2015

My goal is for anything that says "Disconnect" to work/look the same way

I think there can be different degrees of severity. Disconnect that could break something and disconnect that will turn something off. Sharing is an example of the latter. You disconnect Facebook, and nothing breaks, it just prevents you from Publicizing to Facebook automatically in the future. I don't think this warrants a scary style. What I could see happening here, however is a confirmation dialog.

Also, these two screens both use a wonky faux FoldableCard pattern that should also be synced up in a future PR

👍 Totally agree.

@MichaelArestad

This comment has been minimized.

Show comment
Hide comment
@MichaelArestad

MichaelArestad Dec 11, 2015

Contributor

If the icon isn't clarifying anything, we should drop it. In this case, I don't think it's accurate or particularly helpful. I also would suggest removing the scary styles as these are non-destructive changes like Kelly mentioned.

Can you add a screenshot showing both the connect and disconnect buttons at once? How well do they work together?

Contributor

MichaelArestad commented Dec 11, 2015

If the icon isn't clarifying anything, we should drop it. In this case, I don't think it's accurate or particularly helpful. I also would suggest removing the scary styles as these are non-destructive changes like Kelly mentioned.

Can you add a screenshot showing both the connect and disconnect buttons at once? How well do they work together?

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Dec 11, 2015

Contributor

@MichaelArestad These are totally destructive changes. Revoking a connected application means you can't log in to that app with WordPress.com without re-signing in. The sharing connections are pretty non-destructive, though.

Contributor

alternatekev commented Dec 11, 2015

@MichaelArestad These are totally destructive changes. Revoking a connected application means you can't log in to that app with WordPress.com without re-signing in. The sharing connections are pretty non-destructive, though.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Dec 11, 2015

Contributor

@kellychoffman @MichaelArestad They look like this now:

screen shot 2015-12-11 at 3 53 45 pm

Contributor

alternatekev commented Dec 11, 2015

@kellychoffman @MichaelArestad They look like this now:

screen shot 2015-12-11 at 3 53 45 pm

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
Member

kellychoffman commented Dec 13, 2015

👍

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Dec 17, 2015

Contributor

I've updated this PR to look more like #1474:

screen shot 2015-12-17 at 12 04 36 pm

Contributor

alternatekev commented Dec 17, 2015

I've updated this PR to look more like #1474:

screen shot 2015-12-17 at 12 04 36 pm

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Dec 18, 2015

Member

The only issue I have is with the expand icon so close to the disconnect button. This will especially be an issue on smaller screens. I designed the original Sharing "cards" with this in mind, placing the open icon on the left.

Member

kellychoffman commented Dec 18, 2015

The only issue I have is with the expand icon so close to the disconnect button. This will especially be an issue on smaller screens. I designed the original Sharing "cards" with this in mind, placing the open icon on the left.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Dec 18, 2015

Contributor

@kellychoffman Why does the button being important mean it can't be next to expand/collapse? Would it be too easy to click the button on accident?

FoldableCard looks like a SectionHeader when opened, so I feel like we should be using the compact buttons there, fwiw.

Is something like this out of the question, do you think?

untitled-3

Contributor

alternatekev commented Dec 18, 2015

@kellychoffman Why does the button being important mean it can't be next to expand/collapse? Would it be too easy to click the button on accident?

FoldableCard looks like a SectionHeader when opened, so I feel like we should be using the compact buttons there, fwiw.

Is something like this out of the question, do you think?

untitled-3

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

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Jan 4, 2016

Member

Why does the button being important mean it can't be next to expand/collapse? Would it be too easy to click the button on accident?

Yep, thats all I meant.

I think the design above is quite nice. I like the visual separation between the button and the expand/collapse icons.

Member

kellychoffman commented Jan 4, 2016

Why does the button being important mean it can't be next to expand/collapse? Would it be too easy to click the button on accident?

Yep, thats all I meant.

I think the design above is quite nice. I like the visual separation between the button and the expand/collapse icons.

@folletto

This comment has been minimized.

Show comment
Hide comment
@folletto

folletto Jan 8, 2016

Member

👍

Member

folletto commented Jan 8, 2016

👍

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Jan 8, 2016

Member

+1 to non scary disconnect buttons with a separator between it and the expand/collapse icon.

Member

kellychoffman commented Jan 8, 2016

+1 to non scary disconnect buttons with a separator between it and the expand/collapse icon.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 26, 2016

Contributor

Pinging @kellychoffman for a review of the final design here and @dmsnell for a code review.

Contributor

alternatekev commented Jan 26, 2016

Pinging @kellychoffman for a review of the final design here and @dmsnell for a code review.

alternatekev added some commits Dec 10, 2015

changed disconnect links to Button borderless scary
removed gridicon

updated to look like updates made to sharing page, changing the disconnect button to compact and moving the foldable card icon to the right

updated to look like updates made to sharing page, changing the disconnect button to compact and moving the foldable card icon to the right

fixed ellipsis utf embed

centered button a little better

removed scary prop

changed disconnect links to Button borderless scary

removed gridicon

updated to look like updates made to sharing page, changing the disconnect button to compact and moving the foldable card icon to the right

updated to look like updates made to sharing page, changing the disconnect button to compact and moving the foldable card icon to the right

centered button a little better
style updates to expanded-summary
style updates to expanded-summary to match original design

fixed ellipsis utf embed

removed scary prop

changed disconnect links to Button borderless scary

removed gridicon

updated to look like updates made to sharing page, changing the disconnect button to compact and moving the foldable card icon to the right

updated to look like updates made to sharing page, changing the disconnect button to compact and moving the foldable card icon to the right

centered button a little better

updated faked foldable-cards to real foldable-cards

style updates to expanded-summary to match original design
formatting cleanup
formatting fixes

fixes listing of access permissions
Show outdated Hide outdated client/me/connected-application-item/index.jsx
@@ -60,19 +61,18 @@ module.exports = React.createClass( {
},
renderAccessScopeBadge: function() {
var meta = '',
connection = this.props.connection;
var meta = '';

This comment has been minimized.

@dmsnell

dmsnell Jan 26, 2016

Contributor

I kind of like having the variable shorthand here as the array of this.props. seems distracting. A more elegant solution could be taken with destructuring, however:

const { connection: { scope, site } } = this.props;

if ( 'auth' == scope ) …

meta = site.site_name;
@dmsnell

dmsnell Jan 26, 2016

Contributor

I kind of like having the variable shorthand here as the array of this.props. seems distracting. A more elegant solution could be taken with destructuring, however:

const { connection: { scope, site } } = this.props;

if ( 'auth' == scope ) …

meta = site.site_name;

This comment has been minimized.

@alternatekev

alternatekev Jan 26, 2016

Contributor

My only thought here was that it's helpful to know when a variable is being passed through a prop, but it's a small detail either way. I'll update it to work like this.

@alternatekev

alternatekev Jan 26, 2016

Contributor

My only thought here was that it's helpful to know when a variable is being passed through a prop, but it's a small detail either way. I'll update it to work like this.

This comment has been minimized.

@dmsnell

dmsnell Jan 26, 2016

Contributor

That's interesting, as I've not heard anyone say that - what's the benefit to knowing that it has been passed through a prop? Or, what's the disadvantage of having that declaration at the top of the current function (to indicate where it came from)?

@dmsnell

dmsnell Jan 26, 2016

Contributor

That's interesting, as I've not heard anyone say that - what's the benefit to knowing that it has been passed through a prop? Or, what's the disadvantage of having that declaration at the top of the current function (to indicate where it came from)?

Show outdated Hide outdated client/me/connected-application-item/index.jsx
<div>
<h2>
{ this.translate( 'Application Website' ) }
</h2>

This comment has been minimized.

@dmsnell

dmsnell Jan 26, 2016

Contributor

This isn't a big deal, but I prefer squashing this into a single line for clarity:

<h2>{ this.translate( 'Application Website' ) }</h2>
@dmsnell

dmsnell Jan 26, 2016

Contributor

This isn't a big deal, but I prefer squashing this into a single line for clarity:

<h2>{ this.translate( 'Application Website' ) }</h2>
Show outdated Hide outdated client/me/connected-application-item/index.jsx
onClick={ this.recordClickEvent( 'Connected Application Website Link' ) }
target="_blank"
>
{ safeProtocolUrl( this.props.connection.URL ) }

This comment has been minimized.

@dmsnell

dmsnell Jan 26, 2016

Contributor

This is another place where destructuring is nice because it takes out the distracting and overly-specific this.props.. In our render, we care more about a URL or a connection.URL than we do a this.props.connection.URL

const { URL } = this.props.connection;

// or

const { connection: { URL } } = this.props;

// or

const { connection: { URL: connectionUrl } } = this.props;

// or

const { URL: safeUrl } = this.props.connection;
@dmsnell

dmsnell Jan 26, 2016

Contributor

This is another place where destructuring is nice because it takes out the distracting and overly-specific this.props.. In our render, we care more about a URL or a connection.URL than we do a this.props.connection.URL

const { URL } = this.props.connection;

// or

const { connection: { URL } } = this.props;

// or

const { connection: { URL: connectionUrl } } = this.props;

// or

const { URL: safeUrl } = this.props.connection;
Show outdated Hide outdated client/me/connected-application-item/index.jsx
summary: function() {
return( <div>{ this.props.isPlaceholder
? ( <Button compact disabled>{ this.translate( 'Loading…' ) }</Button> )
: ( <Button compact onClick={ this.disconnect }>{ this.translate( 'Disconnect' ) }</Button> ) }</div> );

This comment has been minimized.

@dmsnell

dmsnell Jan 26, 2016

Contributor

We should split this into multiple lines to match our style elsewhere and to make it explicitly clear:

return (
    <div>
        { isPlaceholder
            ? <Button compact disabled></button>
            : <Button compact onClick=></button> }
    </div>
);

// or

return (
    <div>
        { isPlaceholder
            ? <Button compact disabled></button>
            : <Button compact onClick=></button>
        }
    </div>
);
@dmsnell

dmsnell Jan 26, 2016

Contributor

We should split this into multiple lines to match our style elsewhere and to make it explicitly clear:

return (
    <div>
        { isPlaceholder
            ? <Button compact disabled></button>
            : <Button compact onClick=></button> }
    </div>
);

// or

return (
    <div>
        { isPlaceholder
            ? <Button compact disabled></button>
            : <Button compact onClick=></button>
        }
    </div>
);
@dmsnell

This comment has been minimized.

Show comment
Hide comment
@dmsnell

dmsnell Jan 26, 2016

Contributor

@alternatekev one observation (that kind of seems a bit late here) is that your screenshots looked like they were essentially making an accordion, of which we already have a component (albeit without the header buttons). Have we considered using the <Accordion />, updating the <Accordion />, or switching other old code from the <Accordion /> to get rid of it?

Are these altogether different use cases? (it appeared like we'd only want a single <FoldableCard /> open any given time…)

Contributor

dmsnell commented Jan 26, 2016

@alternatekev one observation (that kind of seems a bit late here) is that your screenshots looked like they were essentially making an accordion, of which we already have a component (albeit without the header buttons). Have we considered using the <Accordion />, updating the <Accordion />, or switching other old code from the <Accordion /> to get rid of it?

Are these altogether different use cases? (it appeared like we'd only want a single <FoldableCard /> open any given time…)

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 26, 2016

Contributor

Have we considered using the , updating the , or switching other old code from the to get rid of it?

Accordion seems designed for use specifically in the Sidebar, and defaults to a single module open at a time. These screens never included a single-module limitation, and it doesn't seem like FoldableCard comes with one either. I still think FoldableCard is the right component here. Having more than one open at a time is fine on Sharing and Connected Applications.

Contributor

alternatekev commented Jan 26, 2016

Have we considered using the , updating the , or switching other old code from the to get rid of it?

Accordion seems designed for use specifically in the Sidebar, and defaults to a single module open at a time. These screens never included a single-module limitation, and it doesn't seem like FoldableCard comes with one either. I still think FoldableCard is the right component here. Having more than one open at a time is fine on Sharing and Connected Applications.

@folletto

This comment has been minimized.

Show comment
Hide comment
@folletto

folletto Jan 26, 2016

Member

What @alternatekev said. ;)

And:

I still think FoldableCard is the right component here. Having more than one open at a time is fine on Sharing and Connected Applications.

Having more than one open is most of the time the right thing, overall. ;)

Member

folletto commented Jan 26, 2016

What @alternatekev said. ;)

And:

I still think FoldableCard is the right component here. Having more than one open at a time is fine on Sharing and Connected Applications.

Having more than one open is most of the time the right thing, overall. ;)

@dmsnell

This comment has been minimized.

Show comment
Hide comment
@dmsnell

dmsnell Jan 26, 2016

Contributor

cool. just making sure 😄

Contributor

dmsnell commented Jan 26, 2016

cool. just making sure 😄

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 26, 2016

Contributor

@dmsnell I've gone and made those updates :)

Contributor

alternatekev commented Jan 26, 2016

@dmsnell I've gone and made those updates :)

Show outdated Hide outdated client/me/connected-application-item/index.jsx
import safeProtocolUrl from 'lib/safe-protocol-url';
import analytics from 'analytics';
import Button from 'components/button';
import FoldableCard from 'components/foldable-card';
module.exports = React.createClass( {

This comment has been minimized.

@dmsnell

dmsnell Jan 27, 2016

Contributor

Since we are already rewriting the import statements, we could/should go ahead and rewrite the export too methinks

export default React.createClass( {
    …
} )
@dmsnell

dmsnell Jan 27, 2016

Contributor

Since we are already rewriting the import statements, we could/should go ahead and rewrite the export too methinks

export default React.createClass( {
    …
} )
Show outdated Hide outdated client/me/connected-application-item/index.jsx
@@ -120,116 +111,103 @@ module.exports = React.createClass( {
if ( message ) {

This comment has been minimized.

@dmsnell

dmsnell Jan 27, 2016

Contributor

Looks like we don't need to return anything if message isn't defined, in which case it would be nice if we returned early and nixed the indentation instead…

if ( ! message ) {
    return;
}

return (
    <div></div>
);
@dmsnell

dmsnell Jan 27, 2016

Contributor

Looks like we don't need to return anything if message isn't defined, in which case it would be nice if we returned early and nixed the indentation instead…

if ( ! message ) {
    return;
}

return (
    <div></div>
);
Show outdated Hide outdated client/me/connected-application-item/index.jsx
{ permission.description }
</li>
);
}, this ) }

This comment has been minimized.

@dmsnell

dmsnell Jan 27, 2016

Contributor

It looks like we are sending in this into the map, but since this isn't used inside of the map, it's pretty much unnecessary. In fact, if we wanted to take one further step beyond simply removing this, we could take out some noise and replace this with a fat-arrow function:

{ permissions.map( ( { name, description } ) => (
    <li key={ `permission-${ name }` }>
        { description }
    </li>
) }
@dmsnell

dmsnell Jan 27, 2016

Contributor

It looks like we are sending in this into the map, but since this isn't used inside of the map, it's pretty much unnecessary. In fact, if we wanted to take one further step beyond simply removing this, we could take out some noise and replace this with a fat-arrow function:

{ permissions.map( ( { name, description } ) => (
    <li key={ `permission-${ name }` }>
        { description }
    </li>
) }
@dmsnell

This comment has been minimized.

Show comment
Hide comment
@dmsnell

dmsnell Jan 27, 2016

Contributor

@alternatekev another few comments. Thanks for the updates. I absolutely ❤️ all the 🔻 removed lines of code in this PR 😄

Contributor

dmsnell commented Jan 27, 2016

@alternatekev another few comments. Thanks for the updates. I absolutely ❤️ all the 🔻 removed lines of code in this PR 😄

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 27, 2016

Contributor

@dmsnell done and done

Contributor

alternatekev commented Jan 27, 2016

@dmsnell done and done

@dmsnell

This comment has been minimized.

Show comment
Hide comment
@dmsnell

dmsnell Jan 27, 2016

Contributor

Code looks good @alternatekev!

Contributor

dmsnell commented Jan 27, 2016

Code looks good @alternatekev!

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

Merge pull request #1477 from Automattic/update/connected-apps-buttons
Connected Applications: Update to FoldableCard Pattern

@alternatekev alternatekev merged commit 12bd128 into master Jan 27, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@alternatekev alternatekev deleted the update/connected-apps-buttons branch Jan 27, 2016

@kellychoffman

This comment has been minimized.

Show comment
Hide comment
@kellychoffman

kellychoffman Jan 29, 2016

Member

Late to the party, but I 👍 this merge :)

Member

kellychoffman commented Jan 29, 2016

Late to the party, but I 👍 this merge :)

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