Skip to content
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

Menus: allow easy reverting of changes #4245

Merged
merged 1 commit into from Apr 29, 2016

Conversation

@georgeh
Copy link
Contributor

commented Mar 23, 2016

This PR will address #22 - Menus: allow easy reverting of changes

The goal is to provide a way to revert changes to the menu without having to trigger a full page reload. Right now the only way to cancel your changes is to navigate away and back, which is not intuitive.

The implementation will be to create a MenuRevertButton component and attach it to the Menu component by the Save and Delete buttons. When clicked it will call MenuData.discard() which reloads directly from the server.

screenshot 2016-03-27 21 08 28

Testing

  • Go into Menus
  • Confirm the Cancel button is disabled
  • Make some changes to a menu
  • Click the Cancel button
  • Confirm the changes have been reverted

@georgeh georgeh force-pushed the add/menu-revert-button branch from 23ae088 to 5b57f71 Mar 24, 2016

*/
var React = require( 'react' ),
classNames = require( 'classnames' ),
debug = require( 'debug' )( 'calypso:menus:revert-button' ); // eslint-disable-line no-unused-vars

This comment has been minimized.

Copy link
@gwwar

gwwar Mar 25, 2016

Member

We can use es6 style imports:

import React from 'react';
import classNames from 'classnames';
import debugFactory from 'debug';

const debug = debugFactory( 'calypso:menus:revert-button' );
classNames = require( 'classnames' ),
debug = require( 'debug' )( 'calypso:menus:revert-button' ); // eslint-disable-line no-unused-vars

var MenuRevertButton = React.createClass( {

This comment has been minimized.

Copy link
@gwwar

gwwar Mar 25, 2016

Member

We can use const here :)

}
} );

module.exports = MenuRevertButton;

This comment has been minimized.

Copy link
@gwwar

gwwar Mar 26, 2016

Member

es6 style would be:

export default MenuRevertButton;

Also see: http://exploringjs.com/es6/ch_modules.html

debug = require( 'debug' )( 'calypso:menus:revert-button' ); // eslint-disable-line no-unused-vars

var MenuRevertButton = React.createClass( {
componentWillMount: function() {

This comment has been minimized.

Copy link
@gwwar

gwwar Mar 26, 2016

Member

For any methods in a class, we can also write it as:

componentWillMount() {

vs

componentWillMount: function() {

@gwwar gwwar added the Menus label Mar 26, 2016

},

render: function() {
var hasChanged = this.props.menuData.get().hasChanged,

This comment has been minimized.

Copy link
@gwwar

gwwar Mar 26, 2016

Member

const here too.

} );

return (
<button className={ classes }

This comment has been minimized.

Copy link
@gwwar

gwwar Mar 26, 2016

Member

Perhaps consider using our button component, available in client/components/button.

Also see: https://wpcalypso.wordpress.com/devdocs/design/buttons for how they work in action

@gwwar

This comment has been minimized.

Copy link
Member

commented Mar 26, 2016

👍 works as advertised. I would also suggest removing the primary class from the button since this is not a primary action.

cc @lancewillett @mcsf @folletto for more 👀

@georgeh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2016

@gwwar I updated the class to use the ES6 features you pointed out, and also switched out the DOM for a Button component without the primary class. I updated the screenshot at the top to reflect the new style.

@folletto

This comment has been minimized.

Copy link
Member

commented Mar 28, 2016

Hello!

I agree on the need of something like this, however, we avoided putting that button there because it impairs the use of that part of the UI on mobile – reducing too much the space for the title. See what happens:

screen shot 2016-03-28 at 12 16 44

This problem is even more evident on languages that have longer words for "Save" and "Cancel".

I think we can still add the button if we want, but we need to address the mobile breakpoint. Without changing the design (which is needed, but way beyond the scope here) I see only one potential solution:

  1. At small viewports, we stop showing the title and we just show the pencil icon
  2. Once the icon gets clicked, the whole line gets substituted by the text field, one can type in, and once saved the line goes back to just icon + buttons.

Does it make sense? :)

@georgeh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2016

@folletto Yup, that makes sense. An alternative might be to change "Cancel" to the Cross icon - that seems like it would a) keep the edit icon on the same line and b) be consistent with the delete icon.

I'll play with both solutions and get some screenshots to consider.

Edit: I ruled out the Cross icon because it didn't make any sense unless you already know what it does. Not good UI.

I'm running into a sticking point with the pencil icon wrapping. It's not as simple as adding a mobile breakpoint to hide it, because it's also wrapping at the widths of 660px to 706px. I'm going to try to figure out how to get it to shrink the title instead of wrapping at those widths.

@georgeh georgeh force-pushed the add/menu-revert-button branch 2 times, most recently from cbe6114 to e663d83 Mar 29, 2016

@folletto

This comment has been minimized.

Copy link
Member

commented Mar 29, 2016

Edit: I ruled out the Cross icon because it didn't make any sense unless you already know what it does. Not good UI.

I just opened the thread to say that and then I saw the edit 👍

I'm running into a sticking point with the pencil icon wrapping.

Yeah I recall that being an annoying bit to work with. :|

@georgeh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2016

@folletto I fixed the mobile wrapping issue, but please compare cbe6114 with Automattic/calypso-pre-oss@0ef153c - I believe the max-width: calc(...) + overflow: ellipsis issue is no longer happening but I am having trouble confirming. Do you remember what platform that was happening on?

I tried a few different configurations and took screenshots at a 320px width.

Option 1: Here's what it looks like with just the line-break fixed:
Words and Icon

Option 2: Here's what it looks like if I remove the title:
Icon only

Option 3: I also tried hiding the pencil icon instead of the title with small view ports but it hides the fact that it's editable:
Words only

Also, here's what it looks like when you get into edit mode:
Editing

My vote is for option 1, but my read of your solution is option 2. Do you have a preference? I'll wait to commit my changes until I hear from you.

},

render() {
const hasChanged = this.props.menuData.get().hasChanged;

This comment has been minimized.

Copy link
@mcsf

mcsf Mar 31, 2016

Member

Not important, but this could use ES6 destructuring to make it slightly shorter and more concise.

This comment has been minimized.

Copy link
@georgeh

georgeh Mar 31, 2016

Author Contributor

The only thing I could come up with is const { hasChanged: hasChanged } = this.props.menuData.get(); which is slightly longer and I'm not sure it adds any clarity. Am I missing a cool ES6 feature?

This comment has been minimized.

Copy link
@nb

nb Apr 6, 2016

Member

In ES2015 you can substitute {something: something} with just {something}:

const { hasChanged } = this.props.menuData.get();

This comment has been minimized.

Copy link
@georgeh

georgeh Apr 7, 2016

Author Contributor

TIL! Thanks!

@mcsf

This comment has been minimized.

Copy link
Member

commented Mar 31, 2016

I haven't QA'd, but the changes look good code-wise. They also serve as a painful reminder of how horrible the Menus architecture is. :)

@folletto

This comment has been minimized.

Copy link
Member

commented Mar 31, 2016

My vote is for option 1, but my read of your solution is option 2. Do you have a preference? I'll wait to commit my changes until I hear from you.

I understand why option 1, but gets very tight. To be honest I don't thing we have a real winner here, just varied sub-optimal solutions. For the sake of this PR, and waiting a more proper approach, let's go for Option 1.

Would be nice if possible to make the text are full width in that case, like this:

screen shot 2016-03-31 at 12 38 23

But I would consider this a nice extra and not a blocker to get this PR merged. :)

@georgeh georgeh force-pushed the add/menu-revert-button branch from dd2f67f to f31fae9 Mar 31, 2016

@georgeh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2016

I pushed up changes to lock in Option 1 so hopefully this is good to go. I looked a bit at hiding the other buttons but I didn't want that to block this PR.

@@ -242,6 +243,7 @@ var Menu = React.createClass( {
selectedLocation={ this.props.selectedLocation }
setBusy={ this.props.setBusy }
confirmDiscard={ this.props.confirmDiscard } />
<MenuRevertButton menuData={ this.props.siteMenus } />

This comment has been minimized.

Copy link
@nb

nb Apr 6, 2016

Member

Why do we call this a revert button if it says “Cancel” in the UI?

This comment has been minimized.

Copy link
@georgeh

georgeh Apr 7, 2016

Author Contributor

I thought about that and chose "revert" intentionally. When looking at the UI the word "cancel" is clearer about what it does for the user, but I think that "revert" explains more about what the button does at a code level. I decided that it was better for the code to describe what the code does and the UI to describe what the user wants to accomplish, than to have a generalized term for the code that isn't as clear.

return (
<Button disabled={ ! hasChanged }
onClick={ this.revert }>
{ this.state.reverting ? this.translate( 'Canceling…' ) : this.translate( 'Cancel' ) }

This comment has been minimized.

Copy link
@nb

nb Apr 6, 2016

Member

It’s great that you used the unicode ellipsis character here, instead of three dots 😀 💌

@@ -194,7 +194,7 @@
span {
display: block;
float: left;
max-width: 90%; /* should be less to avoid a bug on touch + <a> tag that auto-closes the area, but ellipsis wouldn't work */
max-width: calc(100% - 20px);

This comment has been minimized.

Copy link
@nb

nb Apr 6, 2016

Member

We might want to add spaces around those parens.

This comment has been minimized.

Copy link
@nb

nb Apr 6, 2016

Member

Hm, I see we’re not terribly consistent with those spaces. Your choice.

This comment has been minimized.

Copy link
@georgeh

georgeh Apr 7, 2016

Author Contributor

That was bugging me so I added them.

return { reverting: false };
},

setReverting() {

This comment has been minimized.

Copy link
@nb

nb Apr 6, 2016

Member

How do you think names like startReverting() and endReverting() would semantically compare to their set* equivalents?

This comment has been minimized.

Copy link
@georgeh

georgeh Apr 7, 2016

Author Contributor

Good question, I hadn't considered those names. reverting and reverted are states, which is what these methods set. start and end seem more like events, which would imply that these methods would emit something. Also, their usage in the code seems more natural with setReverting() and setReverted() than start/end. Based on that reasoning I prefer their current names, but am interested in hearing arguments for start/end.

This comment has been minimized.

Copy link
@gwwar

gwwar Apr 28, 2016

Member

setReverting reads like the method should be passed a boolean value, so something like startReverting() and endReverting() might work better here.

This comment has been minimized.

Copy link
@georgeh

georgeh Apr 29, 2016

Author Contributor

Sold! That totally makes sense, I'll make the updates.

@lancewillett

This comment has been minimized.

Copy link
Member

commented Apr 7, 2016

Sorry for the bot noise... my fault.

@folletto

This comment has been minimized.

Copy link
Member

commented Apr 11, 2016

I played around with making the title 100% width at small widths but don't like how the buttons wrap:

I agree, I don't think it should wrap.

Editing the field should cover the entire line, see the mockup above. :)

@georgeh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2016

@folletto I made some updates to track if the title editor is open, then hide the menu buttons at <480px width. This way when they tap the title on mobile, it will use the full width like in your mockup.

@folletto

This comment has been minimized.

Copy link
Member

commented Apr 12, 2016

Tested on Chrome, Firefox, Safari: 👍

@georgeh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2016

I pulled this back to see if there was a cleaner way to have MenuName communicate its state to Menu, either with an event emitter or with Reflux. Neither seemed to be used very much in this component so I decided to leave the callback prop to pass the state from MenuName to Menu rather that introduce new architecture.

@gwwar

This comment has been minimized.

Copy link
Member

commented Apr 28, 2016

Unless you intend to persist this menu state through multiple page views, what you have here is appropriate. The Redux approach might involve adding an additional subtree to ui. See also: https://github.com/Automattic/wp-calypso/blob/master/docs/our-approach-to-data.md#ui-state

@georgeh georgeh force-pushed the add/menu-revert-button branch from 2f84b0a to 3d7403b Apr 28, 2016

@gwwar

This comment has been minimized.

Copy link
Member

commented Apr 28, 2016

Let's disable the cancel button when we click save. We'll see some unexpected behavior when we do the following:

  1. Make some changes
  2. Click Save
  3. Click Cancel
  4. Full page refresh
@gwwar

This comment has been minimized.

Copy link
Member

commented Apr 28, 2016

👍 Tested on Chrome, FF, IE. @georgeh ping me when that last corner case is cleared up, and I can help you deploy.

Add Cancel button to Menu editor
The only way to revert changes to the menu editor was to navigate away from the
page. This adds a button which will fetch the saved menu from the server. I
used menu-save-button.jsx as a template for the new button.

This uses the Button component instead of DOM <button>.  This way we get any
improvements to that button, plus we get to remove an external dependency
(classNames) at the expense of adding an internal one.

The edit button was wrapping at mobile widths and at 660px to 706px. Mobile is
easy: add a breakpoint. The other one was tougher. The solution was to change
the width of the element to a calc() value that provided space for the button.

This was originally avoided in the pre-oss repo commit 0ef153c because
`text-overflow: ellipsis` did not support `width: calc()`. Browsers appear to
have improved since then. Our new solution is to expand the edit box to the
full width while it is open to make it easier to edit longer menu titles on
narrower viewports.

@georgeh georgeh force-pushed the add/menu-revert-button branch from e24184f to c42a3ec Apr 29, 2016

@georgeh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2016

Thanks for the improvements @gwwar - I've pushed up changes that will disable the Cancel button while a save is in progress.

@georgeh georgeh merged commit 2cc9bf9 into master Apr 29, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@georgeh georgeh deleted the add/menu-revert-button branch Apr 29, 2016

@gwwar

This comment has been minimized.

Copy link
Member

commented Apr 29, 2016

hifive

Fixes #22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.