feat(ApplicationLauncher): Add Application Launcher Component #319

glekner · 2018-04-26T15:15:22Z

fix #184

What:
Add an Application Launcher Component.

Managed to show all the launcher types in one storybook: Grid/List Icons/No Icons with the help of the knobs

Link to Storybook:
Storybook

Additional issues:

Figuring out the Icons names are a real pain, any easy way to do it?
e.g. - applauncher-pf-link-icon pficon pficon-domain - What is the name prop that i need to give to <Icon /> in order to render this icon?

TODO:

Create Tests

sharvit · 2018-04-26T15:26:58Z

src/components/ApplicationLauncher/ApplicationLauncherItem.js

+    >
+      <li className={classes} role="menuitem">
+        <a className="applauncher-pf-link" href="#" onClick={e => onClick(e)}>
+          <i className={icon} aria-hidden="true" />


coveralls · 2018-04-29T08:25:17Z

Pull Request Test Coverage Report for Build 1441

30 of 38 (78.95%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.04%) to 74.679%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/core/src/components/ApplicationLauncher/ApplicationLauncher.js	5	6	83.33%
packages/core/src/components/ApplicationLauncher/Wrappers/ApplicationLauncherWrapper.js	6	7	85.71%
packages/core/src/components/ApplicationLauncher/ApplicationLauncherToggle.js	5	6	83.33%
packages/core/src/components/ApplicationLauncher/ApplicationLauncherItem.js	7	9	77.78%
packages/core/src/components/ApplicationLauncher/Wrappers/StatefulApplicationLauncherWrapper.js	5	8	62.5%

Totals
Change from base Build 1416:	0.04%
Covered Lines:	1747
Relevant Lines:	2141

💛 - Coveralls

glekner · 2018-04-29T08:37:52Z

Added Tests, Fixed Icons
Added a Wrapper component and story

jeff-phillips-18 · 2018-05-11T12:47:10Z

src/components/ApplicationLauncher/ApplicationLauncherToggle.js

+import { Button } from '../Button';
+
+const ApplicationLauncherToggle = ({ onClick, tooltipPlacement }) => {
+  const tooltip = <Tooltip id="tooltip">Application Launcher</Tooltip>;


Application Launcher should be replaced with a passed in prop.

jeff-phillips-18 · 2018-05-11T12:49:51Z

src/components/ApplicationLauncher/ApplicationLauncher.js

+
+    this.state = {
+      showLauncher: false,
+      type: props.type


why add type to state rather than simply using the prop? It should only be mutable by the caller

jeff-phillips-18 · 2018-05-11T12:52:48Z

src/components/ApplicationLauncher/ApplicationLauncher.js

+      {
+        'applauncher-pf dropdown dropdown-kebab-pf': this.state.type === 'list'
+      },
+      { open: this.state.showLauncher }


Would be better to always specify applauncher-pf dropdown dropdown-kebab-pf and add applauncher-pf-block-list if type === grid.

jeff-phillips-18 · 2018-05-11T12:55:06Z

src/components/ApplicationLauncher/ApplicationLauncherItem.js

+  /** Icon Type */
+  icon: PropTypes.string.isRequired,
+  /** App Tooltip */
+  tooltip: PropTypes.string.isRequired,


Should this be required? I'm not sure all items will need a tooltip.

jeff-phillips-18 · 2018-05-11T12:57:29Z

src/components/ApplicationLauncher/Wrappers/ApplicationLauncherWrapper.js

+};
+ApplicationLauncherWrapper.propTypes = {
+  /** Array of App Objects */
+  apps: PropTypes.array.isRequired,


Could we define a shape for this so callers no what fields are required?

glekner · 2018-05-12T18:51:20Z

Applied @jeff-phillips-18 suggestions

jeff-phillips-18 · 2018-05-14T12:55:12Z

packages/core/src/components/ApplicationLauncher/ApplicationLauncher.js

+  /** Children Node */
+  children: PropTypes.node.isRequired,
+  /** tooltipPlacement */
+  tooltipPlacement: PropTypes.string,


Need to take a tooltip as well otherwise it will always be Application Launcher

jeff-phillips-18 · 2018-05-14T13:04:13Z

packages/core/src/components/ApplicationLauncher/Wrappers/ApplicationLauncherWrapper.js

+  /** Grid instead of List (List is Default) */
+  grid: PropTypes.bool,
+  /** Tooltip Placement */
+  tooltipPlacement: PropTypes.string


Should take a tooltip to pass to the ApplicationLauncher to pass to the ApplicationLauncherToggle

glekner · 2018-05-15T07:42:17Z

Fixed Toggle tooltip and added a Shape proptype for the apps array inside the wrapper.

jgiardino · 2018-05-22T15:40:03Z

@patternfly/patternfly-react-ux
Can someone can do a UX review of this component?

lizsurette · 2018-05-22T16:45:15Z

@jgiardino I'll review this afternoon.

lizsurette · 2018-05-22T16:59:10Z

@gilad215 Thanks for this addition! A few comments based on a UX review:

Is there a plan to support both the Horizontal Nav (short masthead) as well? If not at the moment, maybe it would be worth documenting that this is just for use with the tall masthead?
I'm not sure we need the tooltips. These are specified in the base PF implementation.
Clicking outside of the popup should dismiss the popup as it does in the base PF implementation.
There should not be an arrow pointing to the icon when opened. It should just be a straight edge as a typical dropdown would have.

@jgiardino @Rohoover @terezanovotna anything else you guys want to add?

Thanks,
Liz

glekner · 2018-05-23T09:21:03Z

Added a Stateful Wrapper to handle the Open/Close and added a ClickedOutSide functionality
Toggle Tooltip is now optional

Thanks for the review @lizsurette

Since the size of the dropdowns are the same on the different nav bars, I believe the app launcher is supported on both. I see the same classnames of the toggle component for the short and tall masthead.
Tooltips are in the design section, If youre talking about the toggle tooltip, its now optional.

I added a Stateful wrapper and added a ClickedOutSide functionality to it, but its the responsibility of the consumer to handle these kinds of actions.
Removed the Arrow.

jeff-phillips-18 · 2018-05-23T11:33:04Z

@lizsurette Can you open an issue against the design documentation to remove the arrow pointing to the icon when opened from the images included there?

jeff-phillips-18 · 2018-05-24T12:24:57Z

@lizsurette Looks like what is here is correct then. Are you OK with this now?

lizsurette · 2018-05-24T18:14:01Z

@mcarrano Thanks for confirming the correct design, Matt!

@gilad215 @jeff-phillips-18 Yep, this is good to go now from a UX point of view :)

@jeff-phillips-18 I logged the issue in angular-patternfly: patternfly/angular-patternfly#741

jgiardino · 2018-05-25T17:29:54Z

Great work on this, @gilad215! I have a few questions about it, and then some requests listed below…

~~What’s the difference between the two storybooks?~~ Nevermind. I see you described this in your previous comment.
In the pf core example, at 768px, the elements in the masthead are hidden and become available in the vertical navigation menu. Do you know if that behavior is possible with this component? Or should we add a separate issue to capture that?

Requested updates:

Please provide this link in the storybook for the design documentation:
http://www.patternfly.org/pattern-library/application-framework/launcher/#design
The element .applauncher-pf-block-list is missing the class .dropdown, and the <button> is missing .dropdown-toggle. These classes are used in a few different selectors that apply padding to the button.
The button element should include an aria-expanded attribute, set to “true” when the menu is visible and “false” when the menu is hidden.
Inside the button, the current pf core implementation includes several span elements that are missing. These are all the contents of the button in the pf core example:
```


 
 Application Launcher
 
 

```
In this implementation, there's only this one span:
```

```
- Please update the class names so that they match what’s included for core
- Please move the aria-describedby attribute to the parent <a>. This attribute has better support for interactive elements, like <a>.
The roles currently applied to the menu in the pf core example are actually incorrect. The way they’re applied for the pf-react dropdown button are correct.
- <li> should have role=“presentation”
- <a> should have role=“menuitem”
- explanation about these roles
When I hover over menu items, the <li> gets the attribute aria-describedby=“tooltip”, but it should be applied to the <a> instead. The value should also map to the id value of another element, but currently all menu items are referencing the same tooltip id. Could you update the example to show different tooltip text being used? It would be good to verify that this works as expected, but also good to provide an example that's closer to what consumers would be implementing.

jgiardino · 2018-05-25T17:30:58Z

I opened an issue about the incorrect roles (item # 5 in my previous comment): patternfly/patternfly#1063

glekner · 2018-05-27T09:32:21Z

@jgiardino

In the pf core example, at 768px, the elements in the masthead are hidden and become available in the vertical navigation menu. Do you know if that behavior is possible with this component? Or should we add a separate issue to capture that?

Should be possible, in your example the launcher should not be rendered at a certain width, its the responsibility of the consumer to handle these kind of behaviours.

Fixed doc link
Added .dropdown-toggle for the button, This div already has the .dropdown class for me:

Done
Done, but i'm not sure why we need those extra spans, take a look.
Done
I removed the id for the tooltip itself, theres an id for each of the Apps.

@jeff-phillips-18 I also changed the storybook structure to match the PF one, let me know what you think

jgiardino · 2018-05-29T13:48:54Z

Hi @gilad215,

Thanks for making updates requested above! There's just one element that still doesn't match what's included in pf core:

For this element: 
- It still includes a weird set of class names: fa fa- fa fa-th applauncher-pf-icon should just be fa fa-th applauncher-pf-icon
- It still includes aria-describedby="tooltip". It looks like the tooltip for this icon was removed? If that's the case, this attribute that references it should also be removed. If you do keep this attribute, it should be moved to the parent <button> element.

glekner · 2018-06-03T07:34:19Z

@jgiardino fixed the element :)

dabeng · 2018-06-04T03:59:08Z

Hi @gilad215 I find that launcher button will disappear when the width of window is less than 768px.

jgiardino · 2018-06-04T18:00:56Z

packages/core/src/components/ApplicationLauncher/ApplicationLauncherToggle.js

+          onClick={onClick}
+          bsStyle="link"
+          className="nav-item-iconic dropdown-toggle"
+          aria-describedby="tooltip"


Where is this value coming from? This should the the same value that displays as the tooltip id value, but it isn't obvious to me where the tooltip id value is being defined. When I look at the react-bootstrap documentation, they show examples of tooltip like this <Tooltip id={id}> with id as a required prop.

yeah, ill just change it:
overlay={<Tooltip id={tooltip}>{tooltip}</Tooltip>}
in the OverlayTrigger

jgiardino · 2018-06-04T18:29:00Z

@gilad215 I had to build your branch locally to see the updates, and they match what I was expecting. Thanks for the updates! I did have a question about about the tooltip, that I included in an inline comment.

Do you happen to know why the storybook isn't showing the latest updates? (adding @priley86 and @jeff-phillips-18 in case this is an issue with something in the repo, and also for feedback on my next comment...)

Re: @dabeng's comment:

I find that launcher button will disappear when the width of window is less than 768px.

This behavior comes from pf-core css—the css from core will not show the app launcher icon at a width less than 768px.

But in pf-core, the menu items in the app launcher menu will also get added to the vertical nav when the window is less than 768px in width. I'm assuming that this component is intended to support only the toggle and menu that display when the window is greater than 768px. But my question for everyone (@gilad215, @priley86, @jeff-phillips-18, @mturley) is how should the menu items be added to the vertical nav? Would that functionality be added to the vertical nav component? Is that something the consumer would manage? Do we need another issue to capture this aspect of the app launcher behavior?

jeff-phillips-18 · 2018-06-05T10:45:37Z

The items in right of the masthead (app launcher, help, user menu, notification drawer trigger) are meant to be added to the vertical nav at less than 768px. This is described in http://www.patternfly.org/pattern-library/navigation/vertical-navigation/#design in the Responsive States section.

affects: patternfly-react fix patternfly#184 ISSUES CLOSED: patternfly#184

glekner · 2018-06-06T11:32:21Z

@jgiardino changed overlay={<Tooltip id={tooltip}>{tooltip}</Tooltip>} in the OverlayTrigger
Also updated the Storybook

jgiardino · 2018-06-06T14:32:39Z

Awesome! Thanks, @gilad215! No further comments from me. Great job on this!

We also have issue #382 to address getting the app launcher into the vertical nav at smaller window sizes.

jeff-phillips-18

LGTM! Thanks @gilad215!

glekner requested review from jeff-phillips-18 and priley86 April 26, 2018 15:20

glekner changed the title ~~WIP feat(ApplicationLauncher): Add Application Launcher Component~~ WIP: feat(ApplicationLauncher): Add Application Launcher Component Apr 26, 2018

sharvit reviewed Apr 26, 2018

View reviewed changes

glekner force-pushed the feature-applauncher branch from e0a2f88 to e2f217f Compare April 29, 2018 08:20

glekner force-pushed the feature-applauncher branch from e2f217f to a6e73df Compare April 29, 2018 08:29

glekner changed the title ~~WIP: feat(ApplicationLauncher): Add Application Launcher Component~~ feat(ApplicationLauncher): Add Application Launcher Component Apr 29, 2018

glekner force-pushed the feature-applauncher branch 2 times, most recently from e11ae7b to cea99b6 Compare April 29, 2018 09:21

jeff-phillips-18 requested changes May 11, 2018

View reviewed changes

glekner force-pushed the feature-applauncher branch 2 times, most recently from 3059dbe to e876cac Compare May 12, 2018 18:50

glekner force-pushed the feature-applauncher branch from e876cac to 76189cf Compare May 13, 2018 08:12

jeff-phillips-18 requested changes May 14, 2018

View reviewed changes

glekner force-pushed the feature-applauncher branch 2 times, most recently from 0c11d60 to 2196974 Compare May 15, 2018 07:34

jeff-phillips-18 previously approved these changes May 15, 2018

View reviewed changes

glekner added ux review labels May 18, 2018

glekner dismissed jeff-phillips-18’s stale review via 0c2a6a7 May 23, 2018 09:06

glekner force-pushed the feature-applauncher branch from 2196974 to 0c2a6a7 Compare May 23, 2018 09:06

lizsurette added ux approved and removed ux review labels May 24, 2018

lizsurette mentioned this pull request May 24, 2018

Application Launcher dropdown implementation should remove arrow patternfly/angular-patternfly#741

Open

jeff-phillips-18 previously approved these changes May 25, 2018

View reviewed changes

jgiardino added the css review label May 25, 2018

glekner dismissed jeff-phillips-18’s stale review via d80e8eb May 27, 2018 09:21

glekner force-pushed the feature-applauncher branch 2 times, most recently from d80e8eb to cbc761c Compare May 27, 2018 09:31

glekner force-pushed the feature-applauncher branch from cbc761c to 43b797a Compare May 27, 2018 09:42

glekner force-pushed the feature-applauncher branch from 43b797a to a1409c3 Compare June 3, 2018 07:33

jgiardino reviewed Jun 4, 2018

View reviewed changes

feat(ApplicationLauncher): application Launcher Component

2db104a

affects: patternfly-react fix patternfly#184 ISSUES CLOSED: patternfly#184

glekner force-pushed the feature-applauncher branch from a1409c3 to 2db104a Compare June 6, 2018 11:31

jgiardino added css approved and removed css review labels Jun 6, 2018

jeff-phillips-18 approved these changes Jun 7, 2018

View reviewed changes

priley86 approved these changes Jun 14, 2018

View reviewed changes

priley86 merged commit 0334b80 into patternfly:master Jun 14, 2018

Uh oh!

feat(ApplicationLauncher): Add Application Launcher Component #319

feat(ApplicationLauncher): Add Application Launcher Component #319

Uh oh!

Conversation

glekner commented Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1441

💛 - Coveralls

Uh oh!

glekner commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glekner commented May 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glekner commented May 15, 2018

Uh oh!

jgiardino commented May 22, 2018

Uh oh!

lizsurette commented May 22, 2018

Uh oh!

lizsurette commented May 22, 2018

Uh oh!

glekner commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeff-phillips-18 commented May 23, 2018

Uh oh!

jeff-phillips-18 commented May 24, 2018

Uh oh!

lizsurette commented May 24, 2018

Uh oh!

jgiardino commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgiardino commented May 25, 2018

Uh oh!

glekner commented May 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgiardino commented May 29, 2018

Uh oh!

glekner commented Jun 3, 2018

Uh oh!

dabeng commented Jun 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgiardino commented Jun 4, 2018

Uh oh!

jeff-phillips-18 commented Jun 5, 2018

Uh oh!

glekner commented Jun 6, 2018

Uh oh!

jgiardino commented Jun 6, 2018

Uh oh!

jeff-phillips-18 left a comment

Choose a reason for hiding this comment

Uh oh!

glekner commented Apr 26, 2018 •

edited

Loading

coveralls commented Apr 29, 2018 •

edited

Loading

glekner commented Apr 29, 2018 •

edited

Loading

glekner commented May 23, 2018 •

edited

Loading

jgiardino commented May 25, 2018 •

edited

Loading

glekner commented May 27, 2018 •

edited

Loading