Menus: add icons for move / cancel buttons in menu edit #729

sendilkumarn · 2015-11-25T12:35:16Z

Added icons for move / cancel in menu edit also added a new class to Ok button in that page.

lancewillett · 2015-11-25T17:00:33Z

Hi sendilkumarn — thank you for this contribution. Could you please add a before / after screenshot for visual reference? That's really helpful.

Note: we'd also like to use Gridicons instead of Noticons for the icons.

sendilkumarn · 2015-11-26T03:39:18Z

This is the screenshot with noticons, Let me share a screenshot with gridicons if you are okay with this model.

sendilkumarn · 2015-11-26T07:51:37Z

@lancewillett Please find the gridicons sample attached, I also committed the changes

Also, have added tooltip as label for these buttons.

#25

lancewillett · 2015-11-30T16:29:25Z

CC @folletto for a review.

folletto · 2015-11-30T16:40:12Z

Hi! Thanks for your help there!

About issue #25, could you limit this PR to just the switch to Gridicons and the button alignment? The reason is that icon only makes the buttons obscure on meaning, which impacts the usability and effectiveness of the UI. This means that, with a few exceptions (like the trash can icon) due to clarity or reduced usage, we try to always use text labels, or text labels and icon together.

If you want to read more about it, check the article on NNG "Icons Usability" as a starting point.

So, we can proceed to merge this issue if:

The trash gets a Gridicon
The alignment bug from Menus: update icons to Gridicons #25 is fixed
The buttons with labels will stay as labels

Thank you!

sendilkumarn · 2015-11-30T16:48:15Z

@folletto

Taken that into consideration, i added tooltip for those buttons.

If not okay lemme change that and recommit them again

folletto · 2015-11-30T16:52:20Z

Tooltips are not enough unfortunately. Many user don't even know they exist (they are mostly for experts, ironically) and on touch devices they are overall invisible.

sendilkumarn · 2015-11-30T16:53:51Z

Okay let me recommit with those changes

sendilkumarn · 2015-11-30T17:37:42Z

updated

folletto · 2015-11-30T17:55:59Z

Ok, tested on Chrome, Safari, Firefox, Edge, IE11. 👍
Design wise it's ok. :)

If you want to add a nice extra, mark the trash icon with the is-scary class, which makes it red. ;)

sendilkumarn · 2015-11-30T17:57:59Z

Thought of adding that, but somehow decided against it.
Will add that, it does give a nice ui.

sendilkumarn · 2015-11-30T18:20:01Z

folletto · 2015-11-30T18:24:51Z

Just is-scary, no primary. There's already a primary button there. :)

This:

sendilkumarn · 2015-11-30T18:26:46Z

I thought design wise that will be odd. Are you sure that you need something like that?

folletto · 2015-11-30T18:30:09Z

That's our current design pattern for the whole of Calypso.
Regardless, full red would be ovewhelming. :)

sendilkumarn · 2015-11-30T18:32:00Z

folletto · 2015-11-30T18:33:32Z

Design: 👍

sendilkumarn · 2015-12-01T02:13:56Z

@folletto I have also merged the code.

seear · 2015-12-01T11:05:39Z

client/my-sites/menus/menu-editable-item.jsx

+		var className = ( this.props.className || '' ) + ' button',
+        icon =this.props.icon, gridiconButton = "";
+
+    if(icon) {


The whitespace is not quite right here. Take a look at the spacing guidelines.

seear · 2015-12-01T11:13:57Z

The code looks great—just a few minor style issues to fix. In particular, make sure tabs are used to indent lines.

ehg · 2015-12-01T11:33:42Z

client/my-sites/menus/menu-editable-item.jsx

 				onClick: this.remove
 			},
 			{
 				key: 'move',
-				label: this.translate( 'Move', { textOnly: true } ),
+        label: this.translate( 'Move' ),


We need to keep the { textOnly: true } option, otherwise the translator code will wrap the label in an element.

sendilkumarn · 2015-12-01T11:35:38Z

@seear updated the code please advise if something else needs to be changed.

sendilkumarn · 2015-12-01T11:36:00Z

@ehg Thanks, Will update the same.

ehg · 2015-12-01T11:36:22Z

client/my-sites/menus/menu-editable-item.jsx

@@ -24,9 +25,17 @@ var siteMenus = require( 'lib/menu-data' ),
 var Button = React.createClass({
 	render: function() {
 		var className = ( this.props.className || '' ) + ' button';
+    var icon = this.props.icon;


I don't think this var is necessary, we can just use this.props.icon, since it's only used in two places.

seear · 2015-12-01T12:05:14Z

@sendilkumarn we still have spaces here instead of tabs for line indentation, so the lines aren't aligned correctly:

You may need to adjust some settings in your editor?

sendilkumarn · 2015-12-01T12:35:18Z

@seear can you please let me know in which editor you check? I have used Atom / sublime text both shows correctly in editor but not when uploaded

sendilkumarn · 2015-12-01T12:45:00Z

@seear / @ehg Sorry for any inconvenience caused. I just updated with github editor. I think it is coming correctly. Please let me know still any discrepancy.

Again apologies.

seear · 2015-12-01T16:29:17Z

client/my-sites/menus/menu-editable-item.jsx

@@ -24,9 +25,16 @@ var siteMenus = require( 'lib/menu-data' ),
 var Button = React.createClass({
 	render: function() {
 		var className = ( this.props.className || '' ) + ' button';
+    		var gridiconButton = '';
+
+    		if( icon ) {


I'm getting an error here, we likely need to use this.props.icon.

ehg · 2015-12-02T16:06:29Z

This is looking good :) There are still some mixed tabs and spaces, though.

If you're using Atom, it will help a lot to install an .editorconfig plugin and an eslint plugin — which will automatically configure your editor to use tabs, and warn you about coding style violations, respectively.

Sublime also has similar plugins.

sendilkumarn · 2015-12-03T00:49:06Z

@ehg thanks, you want me to change this merge after installing those plugin. In github page the code is neatly aligned.

ehg · 2015-12-03T10:01:27Z

you want me to change this merge after installing those plugin

Yes please! In the GitHub diff it looks fine, but in my editor the alignment is off. I've highlighted the tab characters to illustrate. The warning symbols on the left are from eslint.

sendilkumarn · 2015-12-03T10:34:30Z

For me it is showing no errors now :(

these are the things that I have used.

ehg · 2015-12-03T11:04:22Z

I just installed atom for the first time. These are my packages:

This is what I see after I restart atom:

sendilkumarn · 2015-12-03T11:26:43Z

Thanks a lot @ehg

Really appreciate your patience. Have updated and there was no errors in my atom now.

ehg · 2015-12-03T11:31:02Z

client/my-sites/menus/menu-editable-item.jsx

 		return (
-			<button className={ className } onClick={ this.props.onClick }>
+			<button className={ className } onClick={ this.props.onClick } title={ this.props.title }>


I don't think we're using title here? :)

👍 Removed

ehg · 2015-12-03T11:43:04Z

This looks great! Thanks for fixing some of the other eslint errors :)

Before we merge this, it's good practice to squash the commits, so our history looks clean. You can also edit the commit message to something more accurate/descriptive.

changed icons into text added is-scary to delete button removed is-primary from delete button aligning to style guide added textonly true identation problems identation updates formatting formatted title removed

sendilkumarn · 2015-12-06T06:37:57Z

@ehg Squashed the commits

ehg · 2015-12-17T16:36:24Z

Moving this to #1739

added icons for move / cancel buttons in menu edit

732d462

lancewillett added Menus [Status] In Progress labels Nov 25, 2015

lancewillett changed the title ~~added icons for move / cancel buttons in menu edit~~ Menus: add icons for move / cancel buttons in menu edit Nov 25, 2015

lancewillett added the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Nov 30, 2015

seear reviewed Dec 1, 2015
View reviewed changes

ehg reviewed Dec 1, 2015
View reviewed changes

seear reviewed Dec 1, 2015
View reviewed changes

sendilkumarn force-pushed the noticon-change branch from 24bf1b0 to d6a8a51 Compare December 3, 2015 10:36

ehg reviewed Dec 3, 2015
View reviewed changes

changed to gridicons and alignment fixes

5aa0db4

changed icons into text added is-scary to delete button removed is-primary from delete button aligning to style guide added textonly true identation problems identation updates formatting formatted title removed

sendilkumarn force-pushed the noticon-change branch from e1989cc to 5aa0db4 Compare December 3, 2015 11:55

ehg added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 4, 2015

lancewillett added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Dec 17, 2015

ehg mentioned this pull request Dec 17, 2015

Menus: Update edit form to use Gridicons #1739

Merged

ehg closed this Dec 17, 2015

ehg removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 17, 2015

lancewillett added the OSS Citizen label Dec 19, 2015

Menus: add icons for move / cancel buttons in menu edit #729

Menus: add icons for move / cancel buttons in menu edit #729

Conversation

sendilkumarn commented Nov 25, 2015

lancewillett commented Nov 25, 2015

sendilkumarn commented Nov 26, 2015

sendilkumarn commented Nov 26, 2015

lancewillett commented Nov 30, 2015

folletto commented Nov 30, 2015

sendilkumarn commented Nov 30, 2015

folletto commented Nov 30, 2015

sendilkumarn commented Nov 30, 2015

sendilkumarn commented Nov 30, 2015

folletto commented Nov 30, 2015

sendilkumarn commented Nov 30, 2015

sendilkumarn commented Nov 30, 2015

folletto commented Nov 30, 2015

sendilkumarn commented Nov 30, 2015

folletto commented Nov 30, 2015

sendilkumarn commented Nov 30, 2015

folletto commented Nov 30, 2015

sendilkumarn commented Dec 1, 2015

seear Dec 1, 2015

Choose a reason for hiding this comment

seear commented Dec 1, 2015

ehg Dec 1, 2015

Choose a reason for hiding this comment

sendilkumarn commented Dec 1, 2015

sendilkumarn commented Dec 1, 2015

ehg Dec 1, 2015

Choose a reason for hiding this comment

seear commented Dec 1, 2015

sendilkumarn commented Dec 1, 2015

sendilkumarn commented Dec 1, 2015

seear Dec 1, 2015

Choose a reason for hiding this comment

ehg commented Dec 2, 2015

sendilkumarn commented Dec 3, 2015

ehg commented Dec 3, 2015

sendilkumarn commented Dec 3, 2015

ehg commented Dec 3, 2015

sendilkumarn commented Dec 3, 2015

ehg Dec 3, 2015

Choose a reason for hiding this comment

sendilkumarn Dec 3, 2015

Choose a reason for hiding this comment

ehg commented Dec 3, 2015

sendilkumarn commented Dec 6, 2015

ehg commented Dec 17, 2015