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

Editor: Contact Form: Add hover text to Edit button #7938

Merged
merged 1 commit into from Sep 19, 2016

Conversation

Projects
None yet
6 participants
@nitrajka
Contributor

nitrajka commented Sep 7, 2016

Hi,

I fixed the bug #4116 . I came up with 4 solutions, but decided to use the following one.
I wrapped the <Gridicon> component in a new <div> and styled the <div> with paddings. Exactly like the remove button located in 'components/tinymce/plugins/contact-form/dialog/field-remove-button.jsx' which is on the left side of edit button.
If there wouldn't be paddings, the hover text would appear not in the same line as the hover text for remove button. I added a Popover component to the edit button. If there would be problem with ES6 import which I added, I can rewrite it to es5 or the others rewrite to es6.

@gwwar

This comment has been minimized.

Show comment
Hide comment
@gwwar

gwwar Sep 7, 2016

Member

Hi @nitrajka thanks for taking a stab at this! This issue is actually a bit harder than it appears.

client/components/foldable-card/index.jsx is a generic component and is used in a lot of places so we'll need to take a slightly different approach in this case. If we used the current diff, this means we'll see that edit button in a lot of other places that we didn't expect.

A few approaches:

            <FoldableCard
                summary={ bothButtons }
                expandedSummary={ bothButtons }

cc @retrofox, @rodrigoi and @aduth for second opinions.

Member

gwwar commented Sep 7, 2016

Hi @nitrajka thanks for taking a stab at this! This issue is actually a bit harder than it appears.

client/components/foldable-card/index.jsx is a generic component and is used in a lot of places so we'll need to take a slightly different approach in this case. If we used the current diff, this means we'll see that edit button in a lot of other places that we didn't expect.

A few approaches:

            <FoldableCard
                summary={ bothButtons }
                expandedSummary={ bothButtons }

cc @retrofox, @rodrigoi and @aduth for second opinions.

@nitrajka

This comment has been minimized.

Show comment
Hide comment
@nitrajka

nitrajka Sep 8, 2016

Contributor

Hi @gwwar,
I understood. I'm going to take a look at it. Thanks for review.

Contributor

nitrajka commented Sep 8, 2016

Hi @gwwar,
I understood. I'm going to take a look at it. Thanks for review.

@nitrajka

This comment has been minimized.

Show comment
Hide comment
@nitrajka

nitrajka Sep 8, 2016

Contributor

While reading the documentation (README.md) of components/foldable-card I noticed that there is a possibility to pass the edit-button-component as a prop of FoldableCard. Notice the actionButton and the actionButtonExpanded props of FoldableCard.
So I created new component field-edit-button according to the field-remove-button in the same file, styled it and passed it through as a prop of FoldableCard. I also removed the first commit changes.
I'm sure that there will be a problem with styles of this new component, I'll fix it soon.

@gwwar

Contributor

nitrajka commented Sep 8, 2016

While reading the documentation (README.md) of components/foldable-card I noticed that there is a possibility to pass the edit-button-component as a prop of FoldableCard. Notice the actionButton and the actionButtonExpanded props of FoldableCard.
So I created new component field-edit-button according to the field-remove-button in the same file, styled it and passed it through as a prop of FoldableCard. I also removed the first commit changes.
I'm sure that there will be a problem with styles of this new component, I'll fix it soon.

@gwwar

@gwwar

This comment has been minimized.

Show comment
Hide comment
@gwwar

gwwar Sep 9, 2016

Member

Sounds good. Ping me again when you're ready for another 👀 at the PR

Member

gwwar commented Sep 9, 2016

Sounds good. Ping me again when you're ready for another 👀 at the PR

@nitrajka

This comment has been minimized.

Show comment
Hide comment
@nitrajka

nitrajka Sep 12, 2016

Contributor

I refactored the styles of my new component - FieldEditButton.
This myFieldEditButton, or whichever Gridicon is placed instead, is being rotated (transformed) every time on expand action. Watch example here: https://wpcalypso.wordpress.com/devdocs/design/foldable-card.
And I think, we don't want the pencil-gridicon to rotate after expand.

So my solution is:
If the FoldableCard is expanded, I pass through its actionButtonExpanded prop <FieldEditButton expanded={ true } />. Else I pass through its actionButton prop <FieldEditButton expanded={ false } />. According to this expanded prop of FieldEditButton the classNames package decides, whether to use is-expanded className or not.
I styled this is-expanded class in styles.scss file. If I didn't make the CSS selector so long, other styles would apply on my component.

Now, I would like to rewrite this component in ES6 and refactor classNames but I'm not sure about some things.

  1. I read in the wp-calypso CSS documentation that 'The component fragment matches the folder name of the React component it's providing styles to.'
    I thought that, if I create the className of FieldEditButton similarly to FieldRemoveButton it will be OK. But now I don't think so. I think, the classNames which I'm using are too long, and not appropriate. Sorry, I have no idea, how should the classNames look like in this case. Could you help me, please?
  2. To rewrite it, I need to implement PureRenderMixin somehow. Is it ok to use react-addons-pure-render-mixin package like there https://facebook.github.io/react/docs/pure-render-mixin.html ? Or I'll have to use PureComponent from this library https://github.com/gaearon/react-pure-render , which you are actually using?

Thanks for review @gwwar

Contributor

nitrajka commented Sep 12, 2016

I refactored the styles of my new component - FieldEditButton.
This myFieldEditButton, or whichever Gridicon is placed instead, is being rotated (transformed) every time on expand action. Watch example here: https://wpcalypso.wordpress.com/devdocs/design/foldable-card.
And I think, we don't want the pencil-gridicon to rotate after expand.

So my solution is:
If the FoldableCard is expanded, I pass through its actionButtonExpanded prop <FieldEditButton expanded={ true } />. Else I pass through its actionButton prop <FieldEditButton expanded={ false } />. According to this expanded prop of FieldEditButton the classNames package decides, whether to use is-expanded className or not.
I styled this is-expanded class in styles.scss file. If I didn't make the CSS selector so long, other styles would apply on my component.

Now, I would like to rewrite this component in ES6 and refactor classNames but I'm not sure about some things.

  1. I read in the wp-calypso CSS documentation that 'The component fragment matches the folder name of the React component it's providing styles to.'
    I thought that, if I create the className of FieldEditButton similarly to FieldRemoveButton it will be OK. But now I don't think so. I think, the classNames which I'm using are too long, and not appropriate. Sorry, I have no idea, how should the classNames look like in this case. Could you help me, please?
  2. To rewrite it, I need to implement PureRenderMixin somehow. Is it ok to use react-addons-pure-render-mixin package like there https://facebook.github.io/react/docs/pure-render-mixin.html ? Or I'll have to use PureComponent from this library https://github.com/gaearon/react-pure-render , which you are actually using?

Thanks for review @gwwar

@gwwar

This comment has been minimized.

Show comment
Hide comment
@gwwar

gwwar Sep 12, 2016

Member

To rewrite it, I need to implement PureRenderMixin somehow.

React added PureComponent in 15.3.0, so we can import this via

import import React, { PureComponent } from 'react';

see: #7599 for example usages

Member

gwwar commented Sep 12, 2016

To rewrite it, I need to implement PureRenderMixin somehow.

React added PureComponent in 15.3.0, so we can import this via

import import React, { PureComponent } from 'react';

see: #7599 for example usages

Show outdated Hide outdated client/components/tinymce/plugins/contact-form/dialog/field-edit-button.jsx
import Gridicon from 'components/gridicon';
import Popover from 'components/popover';
export default React.createClass( {

This comment has been minimized.

@gwwar

gwwar Sep 12, 2016

Member

We can write this as either a stateless functional component, or an es6 class that extends PureComponent.

function ContactFormDialogFieldEditButton( { expanded } ) {
// returns the same jsx as what's in the current render() method
}
export default class ContactFormDialogFieldEditButton extends PureComponent {
@gwwar

gwwar Sep 12, 2016

Member

We can write this as either a stateless functional component, or an es6 class that extends PureComponent.

function ContactFormDialogFieldEditButton( { expanded } ) {
// returns the same jsx as what's in the current render() method
}
export default class ContactFormDialogFieldEditButton extends PureComponent {
Show outdated Hide outdated client/components/tinymce/plugins/contact-form/dialog/field-edit-button.jsx
import Popover from 'components/popover';
export default React.createClass( {
displayName: 'ContactFormDialogFieldEditButton',

This comment has been minimized.

@gwwar

gwwar Sep 12, 2016

Member

We use a babel transform that picks this up from the class or function name. We generally don't need to set this unless it needs to be different from the class or function name.

@gwwar

gwwar Sep 12, 2016

Member

We use a babel transform that picks this up from the class or function name. We generally don't need to set this unless it needs to be different from the class or function name.

Show outdated Hide outdated client/components/tinymce/plugins/contact-form/dialog/field-edit-button.jsx
propTypes: {
expanded: PropTypes.bool.isRequired
},

This comment has been minimized.

@gwwar

gwwar Sep 12, 2016

Member

For a stateless function we can add directly:

ContactFormDialogFieldEditButton.propTypes = {
   expanded: PropTypes.bool.isRequired
};

For a class this can be defined as a static property:

static propTypes = {
    expanded: PropTypes.bool.isRequired
}
@gwwar

gwwar Sep 12, 2016

Member

For a stateless function we can add directly:

ContactFormDialogFieldEditButton.propTypes = {
   expanded: PropTypes.bool.isRequired
};

For a class this can be defined as a static property:

static propTypes = {
    expanded: PropTypes.bool.isRequired
}
Show outdated Hide outdated client/components/tinymce/plugins/contact-form/dialog/field-edit-button.jsx
},
render() {
const classes = classNames("editor-contact-form-modal-field__edit", {

This comment has been minimized.

@gwwar

gwwar Sep 12, 2016

Member

We need another space in:

const classes = classNames( "editor-contact-form-modal-field__edit",

You can also see relevant style warnings locally by running

make eslint-branch 

Feel free to ignore existing warnings outside of the new field-edit-button.jsx

@gwwar

gwwar Sep 12, 2016

Member

We need another space in:

const classes = classNames( "editor-contact-form-modal-field__edit",

You can also see relevant style warnings locally by running

make eslint-branch 

Feel free to ignore existing warnings outside of the new field-edit-button.jsx

This comment has been minimized.

@gwwar

gwwar Sep 12, 2016

Member

From the eslint warning, we're expecting a dialog prefix. cc @mtias should we ignore this one, or use something like dialog-editor-contact-form-field__edit?

@gwwar

gwwar Sep 12, 2016

Member

From the eslint warning, we're expecting a dialog prefix. cc @mtias should we ignore this one, or use something like dialog-editor-contact-form-field__edit?

Show outdated Hide outdated client/components/tinymce/plugins/contact-form/style.scss
.editor-contact-form-modal-field__edit-wrapper {
height: 100%;
button {

This comment has been minimized.

@gwwar

gwwar Sep 12, 2016

Member

Do you mind adding another class to the button instead. We can then do something like:

.editor-contact-form-modal-field__edit-wrapper {
    height: 100%;
}
.editor-contact-form-modal-field__edit-wrapper-button {
   width: 100%;
}
@gwwar

gwwar Sep 12, 2016

Member

Do you mind adding another class to the button instead. We can then do something like:

.editor-contact-form-modal-field__edit-wrapper {
    height: 100%;
}
.editor-contact-form-modal-field__edit-wrapper-button {
   width: 100%;
}
Show outdated Hide outdated client/components/tinymce/plugins/contact-form/style.scss
width: 100%;
}
}
.editor-contact-form-modal-field__edit-wrapper.foldable-card__expand .editor-contact-form-modal-field__edit {

This comment has been minimized.

@gwwar

gwwar Sep 12, 2016

Member

We try to avoid this much specificity, but it might not be avoidable in this case.

@gwwar

gwwar Sep 12, 2016

Member

We try to avoid this much specificity, but it might not be avoidable in this case.

Show outdated Hide outdated client/components/tinymce/plugins/contact-form/dialog/field.jsx
expanded={ this.props.isExpanded }
onOpen={ () => this.props.onUpdate( { isExpanded: true } ) }
onClose={ () => this.props.onUpdate( { isExpanded: false } ) }>
onClose={ () => this.props.onUpdate( { isExpanded: false } ) }

This comment has been minimized.

@gwwar

gwwar Sep 12, 2016

Member

Let's try to avoid the function creation during each render, we can do something like:

handleCardOpen() {
    this.props.onUpdate( { isExpanded: true } );
},
handleCardClose() {
    this.props.onUpdate( { isExpanded: false } );
}
//...
render() {
    //...
    <FoldableCard 
            onClose={ this.handleCardClose }
             onOpen={ this.handleCardOpen }
}
@gwwar

gwwar Sep 12, 2016

Member

Let's try to avoid the function creation during each render, we can do something like:

handleCardOpen() {
    this.props.onUpdate( { isExpanded: true } );
},
handleCardClose() {
    this.props.onUpdate( { isExpanded: false } );
}
//...
render() {
    //...
    <FoldableCard 
            onClose={ this.handleCardClose }
             onOpen={ this.handleCardOpen }
}
@gwwar

This comment has been minimized.

Show comment
Hide comment
@gwwar

gwwar Sep 12, 2016

Member

Thanks @nitrajka I tested this and it seems to work well. I left a few notes in case you feel up to rewriting this as a stateless function, or an es6 class.

cc @alisterscott for some help testing

Member

gwwar commented Sep 12, 2016

Thanks @nitrajka I tested this and it seems to work well. I left a few notes in case you feel up to rewriting this as a stateless function, or an es6 class.

cc @alisterscott for some help testing

@gwwar

This comment has been minimized.

Show comment
Hide comment
@gwwar

gwwar Sep 12, 2016

Member

For any translate usages, we can use localize as a higher order component rather than a mixin. The new component wraps the original component, passing all original props plus props to assist in localization.

import { localize } from 'i18n-calypso';
class ContactFormDialogFieldEditButton extends PureComponent {
    render() {
      const { translate } = this.props;
      return ( <div> { translate( 'Hello World!' ); } </div> ); 
    }
}
export default localize( ContactFormDialogFieldEditButton );
Member

gwwar commented Sep 12, 2016

For any translate usages, we can use localize as a higher order component rather than a mixin. The new component wraps the original component, passing all original props plus props to assist in localization.

import { localize } from 'i18n-calypso';
class ContactFormDialogFieldEditButton extends PureComponent {
    render() {
      const { translate } = this.props;
      return ( <div> { translate( 'Hello World!' ); } </div> ); 
    }
}
export default localize( ContactFormDialogFieldEditButton );
@nitrajka

This comment has been minimized.

Show comment
Hide comment
@nitrajka

nitrajka Sep 12, 2016

Contributor

Thanks a lot, @gwwar! :)
As for me, I'm done with the code, ready to merge. Test it.

Contributor

nitrajka commented Sep 12, 2016

Thanks a lot, @gwwar! :)
As for me, I'm done with the code, ready to merge. Test it.

Show outdated Hide outdated client/components/tinymce/plugins/contact-form/dialog/field-edit-button.jsx
<Popover
isVisible={ this.state.showTooltip }
context={ this.refs && this.refs.editField }
onClose={ () => {} }

This comment has been minimized.

@gwwar

gwwar Sep 12, 2016

Member

Pretty minor, but we can either define this outside of render, or use lodash noop

import { noop } from 'lodash';
//...
onClose= { noop }
@gwwar

gwwar Sep 12, 2016

Member

Pretty minor, but we can either define this outside of render, or use lodash noop

import { noop } from 'lodash';
//...
onClose= { noop }
Show outdated Hide outdated client/components/tinymce/plugins/contact-form/dialog/field-edit-button.jsx
};
constructor() {
super();

This comment has been minimized.

@gwwar

gwwar Sep 12, 2016

Member

Let's pass through args to the parent:

super( ...arguments );
@gwwar

gwwar Sep 12, 2016

Member

Let's pass through args to the parent:

super( ...arguments );
Show outdated Hide outdated client/components/tinymce/plugins/contact-form/dialog/field-edit-button.jsx
}
render() {
const { translate } = this.props;

This comment has been minimized.

@gwwar

gwwar Sep 12, 2016

Member

Optional but, since we're destructuring we can also do:

const { expanded, translate } = this.props;
//...
'is-expanded': expanded
@gwwar

gwwar Sep 12, 2016

Member

Optional but, since we're destructuring we can also do:

const { expanded, translate } = this.props;
//...
'is-expanded': expanded
Show outdated Hide outdated client/components/tinymce/plugins/contact-form/dialog/field-edit-button.jsx
ref="editField"
borderless
onMouseEnter={ () => this.setState( { showTooltip: true } ) }
onMouseLeave={ () => this.setState( { showTooltip: false } ) }>

This comment has been minimized.

@gwwar

gwwar Sep 12, 2016

Member

We can similarly write handlers once in the class rather than in render. The tricky part is that we don't have autobinding so we need to set that up in the constructor.

constructor() {
    super( ...arguments );
    this.state = { showTooltip: false };
    this.handleMouseEnter = this.handleMouseEnter.bind( this );
    this.handleMouseLeave = this.handleMouseLeave.bind( this );
}

handleMouseEnter() {
   this.setState( { showTooltip: true } );
}

handleMouseLeave() {
   this.setState( { showTooltip: false } );
}
@gwwar

gwwar Sep 12, 2016

Member

We can similarly write handlers once in the class rather than in render. The tricky part is that we don't have autobinding so we need to set that up in the constructor.

constructor() {
    super( ...arguments );
    this.state = { showTooltip: false };
    this.handleMouseEnter = this.handleMouseEnter.bind( this );
    this.handleMouseLeave = this.handleMouseLeave.bind( this );
}

handleMouseEnter() {
   this.setState( { showTooltip: true } );
}

handleMouseLeave() {
   this.setState( { showTooltip: false } );
}
@gwwar

This comment has been minimized.

Show comment
Hide comment
@gwwar

gwwar Sep 12, 2016

Member

This looks great @nitrajka! 😄 Almost there. I left a few more minor notes.

If you could tidy up these last few comments, and squash a few of your commits, we'll be ready to :shipit:

Member

gwwar commented Sep 12, 2016

This looks great @nitrajka! 😄 Almost there. I left a few more minor notes.

If you could tidy up these last few comments, and squash a few of your commits, we'll be ready to :shipit:

@alisterscott

This comment has been minimized.

Show comment
Hide comment
@alisterscott

alisterscott Sep 13, 2016

Contributor

Thanks for working on this change. I tested this out and it worked well in Chrome, Firefox and Safari (OSX) - the only issue I found was in Safari, whilst the hover text looks great, the Edit button styling no longer displays the same as previously (and the other button)

safari edit hover

Contributor

alisterscott commented Sep 13, 2016

Thanks for working on this change. I tested this out and it worked well in Chrome, Firefox and Safari (OSX) - the only issue I found was in Safari, whilst the hover text looks great, the Edit button styling no longer displays the same as previously (and the other button)

safari edit hover

@nitrajka

This comment has been minimized.

Show comment
Hide comment
@nitrajka

nitrajka Sep 14, 2016

Contributor

Finally,
I refactored the code according to the @gwwar's suggestions and fixed the layout of FieldEditButton so it appears to look the same in Safari, Firefox and Chrome.

@alisterscott

Contributor

nitrajka commented Sep 14, 2016

Finally,
I refactored the code according to the @gwwar's suggestions and fixed the layout of FieldEditButton so it appears to look the same in Safari, Firefox and Chrome.

@alisterscott

@gwwar

This comment has been minimized.

Show comment
Hide comment
@gwwar

gwwar Sep 17, 2016

Member

Hi @nitrajka! The code looks good here 👍 Do you mind squashing down the commits with a single meaningful commit message. I'll be able to merge this for you.

@mtias did you have any unaddressed concerns?

Member

gwwar commented Sep 17, 2016

Hi @nitrajka! The code looks good here 👍 Do you mind squashing down the commits with a single meaningful commit message. I'll be able to merge this for you.

@mtias did you have any unaddressed concerns?

Create FieldEditButton with Popover description
I passed this component through props of FoldableCard in ContactFormDialogField.
@nitrajka

This comment has been minimized.

Show comment
Hide comment
@nitrajka

nitrajka Sep 18, 2016

Contributor

Sorry, I didn't realize that you wanted to squash ALL of the commits into a single commit. I did it. Thank you for the link to the article about commit message. I hope my commit message is meaningful enough. :) @gwwar

Contributor

nitrajka commented Sep 18, 2016

Sorry, I didn't realize that you wanted to squash ALL of the commits into a single commit. I did it. Thank you for the link to the article about commit message. I hope my commit message is meaningful enough. :) @gwwar

@gwwar

This comment has been minimized.

Show comment
Hide comment
@gwwar

gwwar Sep 19, 2016

Member

Awesome, thanks @nitrajka for the contribution!

Member

gwwar commented Sep 19, 2016

Awesome, thanks @nitrajka for the contribution!

@gwwar gwwar merged commit 0914eaa into Automattic:master Sep 19, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment