Enhance frost-detail-tab-actions to display actions labels in popover. #88 #89

goforsatish · 2018-05-14T17:04:56Z

Overview

Enhance frost-detail-tab-actions to display actions labels in popover

Does this PR close an existing issue?

Yes

Summary

Enhance frost-detail-tab-actions to display action label in popover.
Currently, actions labels are displayed below the each action icon that makes action bar look cluttered in case of many actions on action bar as the icon as well as labels take space on action bar for an action. Displaying only actions icons on the action bar will provide more real estate for other icons. The tooltip will display the corresponding actin label on mouseover on each action.

Issue Number(s)

Closes Enhance frost-detail-tab-actions to display actions labels in popover. #88

Screenshots or recordings

Checklist

Semver

This project uses semver, please check the scope of this PR:

#none#
#patch#
#minor#
#major#

Examples:

NONE
- README.md changes
- test additions
- changes to files that are not used by a consuming application (.travis.yml, .gitignore, etc)
PATCH
- backwards-compatible bug fix
  - nothing about how to use the code has changed
  - nothing about the outcome of the code has changed (though it likely corrected it)
- changes to demo app (/tests/dummy/)
MINOR
- adding functionality in a backwards-compatible manner
  - nothing about how used to use the code has changed but using it in a new way will do new things
  - nothing about the outcome of the code has changed without having to first use it in a new way
  - addition of new CSS selectors
  - addition of new ember-hook selectors
MAJOR
- incompatible API change
  - using the code how used to will cease working
  - using the code how used to will have a different outcome
  - any changes to CSS selector names
  - any removal of CSS selectors
  - any changes to ember-hook selectors
  - possibly changes to test helpers (depends on the changes made)
- any changes to the dependencies entry in the package.json file

CHANGELOG

Enhance frost-detail-tab-actions to display actions labels in popover

ccampbel · 2018-05-14T18:27:57Z

Regarding @notmessenger's earlier question, yes this change to add the popover to the vertical action bar is the intended UX.
As mentioned, since this is meant to be a very constrained space with small icons, there isn't much room for text. Therefore, the idea is to use very simple, common icon metaphors for these actions as well as offering a popover with the full action label that appears on hover.
One thing I noticed from the pictures above in this PR, the precise visuals don't match the visual spec:

the color of the popover is incorrect -- it should follow the hover tooltip spec (BG: Grey_2 @ 85% opacity)
the popover should be shifted over slightly such that the triangle portion of it overlaps with the edge of the action strip itself. Again, see the visual design in the link above.

goforsatish · 2018-05-29T13:16:46Z

I have made the changes according to PR comments. Please review .

ccampbel · 2018-05-29T13:21:42Z

Thanks for making the updates. Looks good. UX approved.

goforsatish · 2018-05-29T17:49:10Z

Action bar panel with action icon :

On Action icon hover :

notmessenger · 2018-05-30T14:29:42Z

tests/dummy/app/pods/demo/more-detail-tabs/controller.js

@@ -50,14 +50,14 @@ export default Controller.extend({
  ],

  @readOnly
-  @computed('selectedTab')
+    @computed('selectedTab')


Why the extra space?

notmessenger · 2018-05-30T14:29:46Z

tests/dummy/app/pods/demo/more-detail-tabs/controller.js

  tabContent (selectedTab) {
    const tab = A(this.get('apiTabs')).findBy('label', selectedTab)
    return tab ? get(tab, 'value') : selectedTab
  },

  @readOnly
-  @computed('selectedTab')
+    @computed('selectedTab')


Why the extra space?

notmessenger · 2018-05-30T14:29:51Z

tests/dummy/app/pods/demo/more-detail-tabs/controller.js

@@ -86,7 +86,7 @@ export default Controller.extend({
  queryParams: ['selectedTab', 'selectedSubTab'],

  @readOnly
-  @computed('subtabs', 'selectedTab')
+    @computed('subtabs', 'selectedTab')


Why the extra space?

notmessenger · 2018-05-30T14:30:34Z

Need to merge in latest master branch

notmessenger · 2018-05-30T14:32:57Z

Unrelated to your changes but if you remove the

const Reporter = require('ember-test-utils/reporter')

and

reporter: new Reporter(),

lines from the testem.js file it will resolve your CI build issues.

notmessenger

If I visit http://ciena-frost.github.io/ember-frost-tabs/#/more-detail-tabs?selectedTab=AIS%20profile there are action icons displayed but when standing up your changes from this PR there are none and the console error of Error: Assertion Failed: A component or helper named "frost-popover" could not be found

notmessenger

You have a conflict in your package-lock.json file, likely due to not having merged in latest master first.

notmessenger · 2018-05-30T17:13:12Z

package.json

@@ -50,6 +50,7 @@
    "ember-code-snippet": "1.7.0",
    "ember-disable-prototype-extensions": "^1.1.0",
    "ember-export-application-global": "^1.0.5",
+    "ember-frost-popover": "^11.0.0",


Shouldn't this be a dependency and not a devDependency since it is required by consumers for this codebase to function correctly?

Which then means this needs to be a MAJOR release and not a PATCH

goforsatish · 2018-05-30T17:33:21Z

notmessenger

You are missing updates to the package-lock.json file

notmessenger · 2018-05-30T18:11:44Z

I have posted a screen recording of this new behavior and have posted it to https://youtu.be/JO5ijeNcqak

@ccampbel Is the "becoming larger and smaller again" effect on hover desired? I'm fine with whatever is desired but I think you've only been looking at screenshots and not interactive behavior.

That also seems like a large bottom margin for triggering the popover but that is just my opinion. Do we need to add another icon to the demo to get a better sense?

notmessenger · 2018-05-30T18:13:05Z

@ccampbel @goforsatish the icon has a border on it as being clicked (browser effect) but remains once have finished clicking. Is this desired?

notmessenger

How did you generate the update to the package-lock.json file? The changes within it do not reflect the changes made to the package.json file.

goforsatish · 2018-05-30T18:24:37Z

I generated package-lock.json by running npm install and merged it with the latest code from branch .

notmessenger · 2018-05-30T18:30:07Z

Via ember install -S ember-frost-popover? Because that is how this dependency should have been installed.

goforsatish · 2018-05-30T18:31:41Z

No. I just did npm install.

goforsatish · 2018-05-30T18:40:26Z

"ember-frost-popover" changes are there in package-lock.json. Is there still anything missing ?

notmessenger · 2018-05-30T18:41:39Z

Okay, I see it now. Maybe I was just looking at an outdated version.

goforsatish · 2018-05-31T07:48:30Z

The the icon border has been removed and it doesn't retain now on click of icon.

notmessenger · 2018-05-31T13:00:28Z

@goforsatish if you run the command npm set progress=false && rm -rf node_modules && rm -rf bower_components && npm install && bower cache clean && bower install and then attempt to run either ember s or ember test do they run as expected or do you receive an error of

Error: Cannot find module 'insert-module-globals'

ccampbel · 2018-05-31T13:14:40Z

Hi @notmessenger - thanks for mentioning these details. I have asked Phil Savignac to take a look at these issues. In the meantime (since he's away till Monday)...

RE: bigger & larger sizing on the icon as hover effect. Generally speaking, the hover effect used across the frost components is not to resize, but to show a light blue background colour. I think we should follow suit with that approach.

RE: large bottom margin for triggering the popover. Yes, it does look rather large. This will mainly become an issue if/when another button is added below it. I expect each button should have a same-sized target area for hover & click actions.

RE: leaving artifact in place around after click. This is not the intent. Making it look "pressed" only makes sense in the case where the overall UI has gone into some kind of "mode" that you need to represent to the user. And they might need to press again to leave that mode. In this case, I believe it will always cause another action to occur (e.g. dialog to open) that leaves no ambiguity for the user as to what they need to do next.

goforsatish · 2018-06-01T05:05:15Z

@notmessenger
Yes, I got Error: Cannot find module 'insert-module-globals' on ember s after executing the below commands:
npm set progress=false && rm -rf node_modules && rm -rf bower_components && npm install && bower cache clean && bower install

notmessenger · 2018-06-01T12:00:35Z

@goforsatish Then you need to fix this. The most likely culprit is that your package-lock.json file got messed up.

goforsatish · 2018-06-11T17:06:40Z

Philsavignac · 2018-06-11T17:14:47Z

UX approved

Satish Kumar added 4 commits April 27, 2018 13:44

action bar changes

7d62ae1

action bar changes

65add6a

css changes for action bar

364e0af

Action bar changes

795ca90

goforsatish requested a review from a team as a code owner May 14, 2018 17:04

PR comments

c309e2f

action bar css changes

2ce8649

Satish Kumar added 3 commits May 30, 2018 01:33

css changes

61ed354

css changes

0bc183a

action icons css changes

8b51cc2

notmessenger reviewed May 30, 2018

View reviewed changes

notmessenger suggested changes May 30, 2018

View reviewed changes

Satish Kumar added 2 commits May 30, 2018 22:00

action icon styling changes

3a03542

PR Comments

0133401

goforsatish requested a review from a team as a code owner May 30, 2018 17:08

notmessenger suggested changes May 30, 2018

View reviewed changes

Satish Kumar added 3 commits May 30, 2018 22:44

package lock

052e3cb

package lock file

606e83b

dependencies update

be821b8

Merge branch 'master' into frost-details-action-enhancement

7b05491

notmessenger suggested changes May 30, 2018

View reviewed changes

package locked

51fa075

package locked

a97c86a

notmessenger suggested changes May 30, 2018

View reviewed changes

Satish Kumar added 2 commits May 31, 2018 00:24

Enabled create action

687b00a

action icons style changes

a2e85b3

Satish Kumar added 2 commits June 6, 2018 10:27

package.json and package-lock.json changes

1824186

PR comments

3cee8ee

notmessenger approved these changes Jun 11, 2018

View reviewed changes

notmessenger merged commit 96eafdc into ciena-frost:master Jun 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance frost-detail-tab-actions to display actions labels in popover. #88 #89

Enhance frost-detail-tab-actions to display actions labels in popover. #88 #89

goforsatish commented May 14, 2018 •

edited

ccampbel commented May 14, 2018

goforsatish commented May 29, 2018

ccampbel commented May 29, 2018 •

edited

goforsatish commented May 29, 2018

notmessenger May 30, 2018

notmessenger May 30, 2018

notmessenger May 30, 2018

notmessenger commented May 30, 2018

notmessenger commented May 30, 2018

notmessenger left a comment

notmessenger left a comment

notmessenger May 30, 2018

notmessenger May 30, 2018

goforsatish commented May 30, 2018

notmessenger left a comment

notmessenger commented May 30, 2018

notmessenger commented May 30, 2018

notmessenger left a comment

goforsatish commented May 30, 2018 •

edited

notmessenger commented May 30, 2018 •

edited

goforsatish commented May 30, 2018

goforsatish commented May 30, 2018

notmessenger commented May 30, 2018

goforsatish commented May 31, 2018

notmessenger commented May 31, 2018

ccampbel commented May 31, 2018 •

edited

goforsatish commented Jun 1, 2018 •

edited

notmessenger commented Jun 1, 2018

goforsatish commented Jun 11, 2018

Philsavignac commented Jun 11, 2018

Enhance frost-detail-tab-actions to display actions labels in popover. #88 #89

Enhance frost-detail-tab-actions to display actions labels in popover. #88 #89

Conversation

goforsatish commented May 14, 2018 • edited

Overview

Does this PR close an existing issue?

Summary

Issue Number(s)

Screenshots or recordings

Checklist

Semver

CHANGELOG

ccampbel commented May 14, 2018

goforsatish commented May 29, 2018

ccampbel commented May 29, 2018 • edited

goforsatish commented May 29, 2018

notmessenger May 30, 2018

Choose a reason for hiding this comment

notmessenger May 30, 2018

Choose a reason for hiding this comment

notmessenger May 30, 2018

Choose a reason for hiding this comment

notmessenger commented May 30, 2018

notmessenger commented May 30, 2018

notmessenger left a comment

Choose a reason for hiding this comment

notmessenger left a comment

Choose a reason for hiding this comment

notmessenger May 30, 2018

Choose a reason for hiding this comment

notmessenger May 30, 2018

Choose a reason for hiding this comment

goforsatish commented May 30, 2018

notmessenger left a comment

Choose a reason for hiding this comment

notmessenger commented May 30, 2018

notmessenger commented May 30, 2018

notmessenger left a comment

Choose a reason for hiding this comment

goforsatish commented May 30, 2018 • edited

notmessenger commented May 30, 2018 • edited

goforsatish commented May 30, 2018

goforsatish commented May 30, 2018

notmessenger commented May 30, 2018

goforsatish commented May 31, 2018

notmessenger commented May 31, 2018

ccampbel commented May 31, 2018 • edited

goforsatish commented Jun 1, 2018 • edited

notmessenger commented Jun 1, 2018

goforsatish commented Jun 11, 2018

Philsavignac commented Jun 11, 2018

goforsatish commented May 14, 2018 •

edited

ccampbel commented May 29, 2018 •

edited

goforsatish commented May 30, 2018 •

edited

notmessenger commented May 30, 2018 •

edited

ccampbel commented May 31, 2018 •

edited

goforsatish commented Jun 1, 2018 •

edited