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

Framework: Add new component: RemoveButton #810

Merged
merged 13 commits into from Dec 1, 2015

Conversation

Projects
None yet
4 participants
@alternatekev
Contributor

alternatekev commented Nov 25, 2015

screen shot 2015-11-09 at 2 51 52 pm

screen shot 2015-11-09 at 2 51 58 pm

As part of the process of standardizing the Trash, Remove, Delete, Disconnect, & Deactivate actions into one cohesive pattern, I've built a new basic component to handle the display of the buttons themselves. The documentation I've written for this component looks like this:

RemoveButton

This component is used to implement dang sweet remove/trash/delete/disconnect/deactivate buttons.

How to use:

var RemoveButton = require( 'components/remove-button' );

render: function() {
    return (
        <RemoveButton compact scary disabled icon={ 'remove | trash | delete | deactivate | disconnect' } >Delete me!</RemoveButton>
    );
}

Props

  • compact: (bool) whether the button is compact or not.
  • scary: (bool) whether the button has modified styling to warn users (red text & icon).
  • disabled: (bool) whether the button should be in the disabled state.
  • icon: (string) a text identifier for what kind of button you want to show. Displays remove by default. Accepts remove, trash, delete, disconnect, deactivate.

Some considerations & caveats:

  • The naming convention for the icon prop is probably not correct. Type was too generic and got forwarded verbatim into the HTML anyway. This is just a pass-through to Gridicon for the most part, with a mapping of the words we use on the screen to the correct icon names. This way the correct icon is handled for the developer/designer if the action is Remove.
  • This button has mostly the same props as the Button component with the exceptions of href and is-link because they didn't seem valuable to me. They could be easily re-implemented.
  • This is the first step toward me being able to implement this in various place. I was planning on starting with /me because it seemed like low-hanging fruit and Mercury people could perhaps support code-reviews on that.
  • I didn't extend the actual Button component per @MichaelArestad's advice to implement a new component instead. This allowed me to simplify the icon parameters that the button expects, but since I implemented similar usage patterns (compact, scary, disabled, etc) it should be easy to adopt.
  • If no text is given in the Component tag, it'll simply render the icon. I don't figure that the icon-less version of this is useful to anyone since it's not part of the Remove pattern anyway_ *Just use a regular Button with scary on it for an icon-less remove button, since we want an outline there *_
  • If an outer button style is desired, simply use <Button scary />
Show outdated Hide outdated client/components/remove-button/index.jsx
};
},
getIconAssignments(){

This comment has been minimized.

@ebinnion

ebinnion Dec 1, 2015

Contributor

This empty method should be removed.

@ebinnion

ebinnion Dec 1, 2015

Contributor

This empty method should be removed.

This comment has been minimized.

@alternatekev

alternatekev Dec 1, 2015

Contributor

Fixed.

@alternatekev

alternatekev Dec 1, 2015

Contributor

Fixed.

Show outdated Hide outdated client/components/remove-button/index.jsx
'disconnect': 'link-break',
'deactivate': 'cross',
'delete': 'trash',
};

This comment has been minimized.

@ebinnion

ebinnion Dec 1, 2015

Contributor

The formatting is off in this section with tab width set to both 4 and 8 spaces. We should probably either leave a single space after the : or align the values vertically.

@ebinnion

ebinnion Dec 1, 2015

Contributor

The formatting is off in this section with tab width set to both 4 and 8 spaces. We should probably either leave a single space after the : or align the values vertically.

This comment has been minimized.

@alternatekev

alternatekev Dec 1, 2015

Contributor

Fixed.

@alternatekev

alternatekev Dec 1, 2015

Contributor

Fixed.

Show outdated Hide outdated client/components/remove-button/index.jsx
* External dependencies
*/
import React from 'react';
import assign from 'lodash/object/assign';

This comment has been minimized.

@ebinnion

ebinnion Dec 1, 2015

Contributor

Let's remove this since we don't use it anywhere in this component.

@ebinnion

ebinnion Dec 1, 2015

Contributor

Let's remove this since we don't use it anywhere in this component.

This comment has been minimized.

@alternatekev

alternatekev Dec 1, 2015

Contributor

Fixed.

@alternatekev

alternatekev Dec 1, 2015

Contributor

Fixed.

Show outdated Hide outdated client/components/remove-button/index.jsx
var iconSize = 12;
} else {
var iconSize = 24;
}

This comment has been minimized.

@ebinnion

ebinnion Dec 1, 2015

Contributor

All var declarations should be at the top of the function scope.

We could rewrite this as let iconSize = this.props.compact ? 12 : 24; perhaps.

@ebinnion

ebinnion Dec 1, 2015

Contributor

All var declarations should be at the top of the function scope.

We could rewrite this as let iconSize = this.props.compact ? 12 : 24; perhaps.

This comment has been minimized.

@alternatekev

alternatekev Dec 1, 2015

Contributor

Fixed.

@alternatekev

alternatekev Dec 1, 2015

Contributor

Fixed.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Dec 1, 2015

Contributor

This still renders the components just fine in the devdocs so it looks good to me.

Contributor

alternatekev commented Dec 1, 2015

This still renders the components just fine in the devdocs so it looks good to me.

);
},
renderButtons: function() {

This comment has been minimized.

@ebinnion

ebinnion Dec 1, 2015

Contributor

Formatting is off in this block as well. Also, note in the below screenshot that several of the lines are using spaces for indentation.

screen shot

@ebinnion

ebinnion Dec 1, 2015

Contributor

Formatting is off in this block as well. Also, note in the below screenshot that several of the lines are using spaces for indentation.

screen shot

This comment has been minimized.

@ebinnion

ebinnion Dec 1, 2015

Contributor

I just pushed up a fix for this.

@ebinnion

ebinnion Dec 1, 2015

Contributor

I just pushed up a fix for this.

Show outdated Hide outdated client/components/remove-button/style.scss
&.hidden {
display: none;
}
&.is-compact {

This comment has been minimized.

@ebinnion

ebinnion Dec 1, 2015

Contributor

There are two spaces indentation here instead of a tab.

@ebinnion

ebinnion Dec 1, 2015

Contributor

There are two spaces indentation here instead of a tab.

This comment has been minimized.

@alternatekev

alternatekev Dec 1, 2015

Contributor

@ebinnion My editor must be screwed up, I could use some help fixing that because it doesn't look that way to me.

@alternatekev

alternatekev Dec 1, 2015

Contributor

@ebinnion My editor must be screwed up, I could use some help fixing that because it doesn't look that way to me.

This comment has been minimized.

@alternatekev

alternatekev Dec 1, 2015

Contributor

Fixed this.

@alternatekev

alternatekev Dec 1, 2015

Contributor

Fixed this.

@ebinnion

This comment has been minimized.

Show comment
Hide comment
@ebinnion

ebinnion Dec 1, 2015

Contributor

I rebased and removed some stray commits from a previous branch. I also made some other changes to fix some formatting and simplify the documentation. If you're fine with my changes, then 🚢

Contributor

ebinnion commented Dec 1, 2015

I rebased and removed some stray commits from a previous branch. I also made some other changes to fix some formatting and simplify the documentation. If you're fine with my changes, then 🚢

alternatekev added a commit that referenced this pull request Dec 1, 2015

Merge pull request #810 from Automattic/add/remove-button-component
Framework: Add new component: RemoveButton

@alternatekev alternatekev merged commit 4149b5d into master Dec 1, 2015

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@ebinnion ebinnion deleted the add/remove-button-component branch Dec 1, 2015

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Dec 7, 2015

Member

Sorry it took me some time to see this one. There's a couple things I'd like to address:

  • Been thinking more about it and absorbing this (and AddNewButton) into Button seem like the best path indeed.
  • The svg icons look to be distorted with width 26px at the moment?
  • Absorbing can reduce the amount of CSS and duplicated props.
Member

mtias commented Dec 7, 2015

Sorry it took me some time to see this one. There's a couple things I'd like to address:

  • Been thinking more about it and absorbing this (and AddNewButton) into Button seem like the best path indeed.
  • The svg icons look to be distorted with width 26px at the moment?
  • Absorbing can reduce the amount of CSS and duplicated props.
import React from 'react';
import classNames from 'classnames';
import noop from 'lodash/utility/noop';
import Gridicon from 'components/gridicon';

This comment has been minimized.

@mtias

mtias Dec 7, 2015

Member

This is internal :)

@mtias

mtias Dec 7, 2015

Member

This is internal :)

// ==========================================================================
.button-remove {
color: darken( $gray, 10% );;

This comment has been minimized.

@mtias

mtias Dec 7, 2015

Member

double ;

@mtias

mtias Dec 7, 2015

Member

double ;

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