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

Application launcher enhancements #3371

Merged
merged 12 commits into from Dec 16, 2019

Conversation

@kmcfaul
Copy link
Contributor

kmcfaul commented Dec 3, 2019

What: Closes #3227

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Dec 3, 2019

@mcarrano Regarding the search/filtering enhancement, what is the expected behavior when no results are found? What is the expected behavior when no items in a group are found? Should filtering apply to the favorites list?

Currently, groups are still displayed even when no results are found within them, and the favorites list is also filtered as a side effect of passing in the filtered results.

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Dec 3, 2019

PatternFly-React preview: https://patternfly-react-pr-3371.surge.sh

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 3, 2019

Codecov Report

Merging #3371 into master will decrease coverage by 0.33%.
The diff coverage is 28.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3371      +/-   ##
==========================================
- Coverage   67.49%   67.15%   -0.34%     
==========================================
  Files         897      903       +6     
  Lines       25158    25417     +259     
  Branches     2181     2230      +49     
==========================================
+ Hits        16980    17069      +89     
- Misses       7162     7326     +164     
- Partials     1016     1022       +6
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.28% <ø> (ø) ⬆️
#patternfly4 64.31% <28.76%> (-0.64%) ⬇️
Impacted Files Coverage Δ
...nfly-4/react-core/src/components/Select/Select.tsx 64.28% <0%> (ø) ⬆️
...ts/ApplicationLauncher/ApplicationLauncherItem.tsx 25% <10.52%> (ø)
...eact-core/src/components/Dropdown/DropdownItem.tsx 71.42% <100%> (+3%) ⬆️
...s/patternfly-4/react-core/src/helpers/constants.ts 100% <100%> (ø) ⬆️
...e/src/components/Dropdown/InternalDropdownItem.tsx 64.21% <26.66%> (-8.16%) ⬇️
...onents/ApplicationLauncher/ApplicationLauncher.tsx 51.85% <33.92%> (-36.61%) ⬇️
...ckages/patternfly-4/react-core/src/helpers/util.ts 50.53% <4.54%> (-8.44%) ⬇️
...eact-core/src/components/Dropdown/DropdownMenu.tsx 54.23% <60%> (+2.38%) ⬆️
...ponents/ApplicationLauncher/ApplicationLauncher.js 64.13% <0%> (-26.53%) ⬇️
...nts/ApplicationLauncher/ApplicationLauncherItem.js 46.57% <0%> (-10.33%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd3cd3d...d730c10. Read the comment docs.

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Dec 3, 2019

Regarding the search/filtering enhancement, what is the expected behavior when no results are found? What is the expected behavior when no items in a group are found? Should filtering apply to the favorites list? Currently, groups are still displayed even when no results are found within them, and the favorites list is also filtered as a side effect of passing in the filtered results.

@kmcfaul Thanks for raising that question. It was definitely missing from the design. I want to loop in @maryshak1996 as she was the primary designer. I'm thinking that maybe we just insert a string "No items found" under each category header with no hits, but am open to ideas. The other thing that I wondered about was the behavior of refreshing the menu when things are added and removed from the Favorites set. While this is implemented as designed, it does feel disorienting to me to have the menu refresh (and items move) when the user toggles a favorites tag. I'm wondering if the list of Favorites should only include favorites as loaded when the menu was opened and not update dynamically. Mary, Interested in your thoughts.

@maryshak1996

This comment has been minimized.

Copy link

maryshak1996 commented Dec 3, 2019

@kmcfaul @mcarrano those are really good points to bring up
In regard to having newly-favorited items getting added to the top 'Favorites' section, I don't personally find it disorienting, but I also don't have very strong feelings around it -- @mcarrano I'm open to hearing any alternatives!
And, to the question about the empty state, I would agree with Matt's thought for sure. @kmcfaul I mocked some references here:
Search finds no application names
image
Search finds applications in the same group
image
Search finds applications in multiple groups
image
**Note: I saw in your implementation that the search retains the groupings even when there's nothing in the group -- is this something that can change (screenshot of what I'm referring to below), so that there isn't so many unneeded lines?
Screen Shot 2019-12-03 at 3 47 42 PM

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Dec 3, 2019

Thanks @maryshak1996 . I don't feel that strongly about the favoriting behavior either. @Chart @bmignano can you also take a look at the behavior for toggling favorites off and on and let us know if this is what you expected? Agree that clarifying the filtering behavior is more critical.

@bmignano

This comment has been minimized.

Copy link

bmignano commented Dec 4, 2019

This favoriting behavior makes sense to me – I don't think it's too disorienting since the movement is a direct result of an action the user takes. If we waited to load the favorites until the menu was closed and reopened I worry it'd be more confusing as the feedback would be delayed upon clicking a star.
As for filtering, I'm not sure we need to maintain headers when no items in specific groups are found. I think Mary's screenshots above are a good approach to filtering - only show the groups when an item matches the filter, and if no items match, show "No applications found".

@jessiehuff

This comment has been minimized.

Copy link
Contributor

jessiehuff commented Dec 5, 2019

This looks awesome! It's a cool addition to our app launcher. :) I know you're still working out specifics, but I just wanted to put a few things on your radar.

It looks like the input field isn't accessible through keyboard interaction and the favorites option isn't accessible by keyboard or screen reader. I'll try to research suggestions on the best way to handle it, but I imagine it would work similar to other dropdown components.

Currently I believe our typeahead select puts focus on the input field first instead of the first menu item, and when your focus is in the menu, shift + tab takes you to the input field. (However, I know that we've been in discussion about the best way to handle typeahead select so I'm interested in others' opinions here.)

In terms of the favorites option, I would think it would be treated similarly to other menu items in using the arrow keys, but I'd have to do some research to see if there is a recommended way for this use case. Interested to hear your thoughts on that interaction @mcarrano.

For the screen reader, I'll have to play with it a bit to see what would allow the favorites icon to be announced. I imagine it needs some kind of discernible text like an aria-label so the screen reader can see it, but I'd have to test to verify that theory.

Thanks for working on this, Katie!!

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Dec 5, 2019

For the screen reader, I'll have to play with it a bit to see what would allow the favorites icon to be announced. I imagine it needs some kind of discernible text like an aria-label so the screen reader can see it, but I'd have to test to verify that theory.

FWIW, I looked at google and they change the aria-label between "starred" and "not starred". Not sure if that's a good way to announce, or to announce it more as the action you'll perform, like "favorite" and "unfavorite"?

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Dec 5, 2019

I'll add in the aria-label swapping and review some of the keyboard behavior. As ApplicationLauncher wraps Dropdown it should be there, but I think the search input might need to be wrapped in an ApplicationLauncherItem to get properly picked up by Dropdown's keyboard interactions.

I'm also working on refining the empty state behavior, removing the last group's separator and removing groups that contain no filtered items. @mcoker @maryshak1996 What is the structure of the "No results found" visual? I currently have a placeholder in an unstyled div.

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Dec 5, 2019

@jessiehuff After experimenting with keyboard behavior, the desired interactions may be more complicated to implement. Since ApplicationLauncher wraps Dropdown, it has Dropdown's behavior which doesn't quite work for this use case as we are passing in custom elements and expanding DropdownItem beyond it's original structure.

The search input can be targeted using arrow keys if it is wrapped in an ApplicationLauncherItem, which gets the context used to build out Dropdown's ref collection, but unfortunately loses its functionality as an input field. I haven't found a clean way to insert a custom element into Dropdown's ref collection yet, but am trying a couple different things.

The favorites button exists within the DropdownItem li - where the ref sits for the keyboard to target. However there isn't a way yet to tab into or select the secondary element, and it defaults to targeting the first element, which is the button/anchor that contains the text. This issue also extends to the external link feature, which sits inside the primary element but may point elsewhere. How do we want to handle this? It might be possible to build out a ref collection of each targetable element instead of just the item itself, and then arrow keys would take the user through each item.

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Dec 5, 2019

What is the structure of the "No results found" visual? I currently have a placeholder in an unstyled div.

@maryshak1996 should we just use an empty state there?

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Dec 5, 2019

Updated with current 'no results' div and logic to remove groups/separators when a middle group happens to be the last group when filtered.

Added a small fix for keyboard to prevent it from failing when there is an undefined ref in the ref collection (as is the case when the search input div is present).

Copy link
Contributor

tlabaj left a comment

You also need to update the demo-app and the cypress tests.

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Dec 9, 2019

Updated with pr feedback and added cypress/TS demo

@kmcfaul kmcfaul force-pushed the kmcfaul:application-launcher-enhancements branch from 52c130e to d7e9f66 Dec 11, 2019
@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Dec 11, 2019

Updated with keyboard navigation changes to support left/right on favorites menu. Should not be breaking because keyboard handling is internal. I've tested Dropdown, Select, OptionsMenu as well to ensure that keyboard interaction still works.

Added some props to allow the custom search input to be parsed and added to the refs list in Dropdown properly for keyboard interaction. Renamed additionalChildren to additionalChild as it expects a single node. This node may have children, but as far as refs, the outermost node is assigned.

@jessiehuff @tlabaj

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Dec 11, 2019

@maryshak1996 @mcoker Any update on the empty state visual?

@maryshak1996

This comment has been minimized.

Copy link

maryshak1996 commented Dec 11, 2019

It looks great to me, @kmcfaul :)

/** Flag indicating if the item is favorited */
isFavorite?: boolean;
/** Aria label text for favoritable button when favorited */
ariaIsFavorite?: string;

This comment has been minimized.

Copy link
@tlabaj

tlabaj Dec 12, 2019

Contributor

Maybe append Label to the end of these so it is more obvious that it is a label and not a boolean.

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Dec 12, 2019

Overall this looks great to me @kmcfaul. I noticed there is a bit of a structure difference in the HTML, mainly in the __menu-item class. In react, there is a __menu-item-text in some of the app launcher examples. I don't see any style in core for the __menu-item-text element. Do you know what that is for?

And __menu-item-external-icon is a <span> instead of a class on the icon itself in core.

I don't think either of these are breaking but they don't quite match.

Also looks like the external icon in the sections and icons example is off. Looks like this is because the .pf-m-link class is being applied to those links. For reference, the .pf-m-link class should only apply to the main link/action in a __menu-wrapper element as a means to space the link/action from whatever may be placed beside it.

Screen Shot 2019-12-12 at 9 45 03 AM

I would also like to fix this spacing between the search divider and the first element, and have opened a core issue patternfly/patternfly-next#2514

Screen Shot 2019-12-12 at 9 31 49 AM

Screen Shot 2019-12-12 at 9 41 13 AM

Screen Shot 2019-12-12 at 9 41 19 AM

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Dec 12, 2019

@mcoker

I'm not sure what __menu-item-text is used for. I can remove it if it doesn't do anything.

__menu-item-external-icon only works when it's applied to the outer span from what I tested, not when its applied to the icon itself. edit: I take that back, it was a different change that did this, I will update.

Fixed the application of the .pf-m-link and .pf-m-external.

The lack of space could be because the item is still wrapped in a group, but the group doesn't have a title.

Copy link
Contributor

jessiehuff left a comment

This looks awesome, Katie! :)
One quick thing you can add is an alt attribute to each of the images. Since they're really just for decoration, I'd recommend using alt=""

Jenn and I were discussing some considerations for future enhancements, and we came up with a few points to think about potentially for a future PR:

  • Favorite button - consider using aria-pressed, and not changing the label (i.e. the aria-pressed attribute would communicate state of button). Also, the label we use for the button should match the label we use for the section that it gets added to when pressed. So I suggest saying “Favorite” as the button label, which would be announced like “Favorite button, unpressed”
  • Filter input - There should be some feedback to the user about how the list changes based on the value they specify. This is worth testing with using either an aria-live region or aria-describedby attribute on the input (this would require prototyping and testing)
  • Section headers - these are not accessible to screen reader users. One suggestion for this is to include aria-labelledby=“id of menu item, id of header” so that the header is communicated with each menu action. There might be other ways of handling this, though.
  • Roles and semantics - I’m pretty sure the roles and tabindex attributes are off. I also think that menuitem is not the best role for this, because it doesn’t provide the correct semantics to the screen reader user (this is the purpose of the role attribute, i.e. what is the thing so that sr users know how to interact with the thing). We need to communicate that this menu is actually a grid, so that they are aware that there are two columns that they can access.
  • Group 2 is reading as "group 12" in voiceover. I wonder if it was saying “group 12" because it perceives the links as being in a group with 12 items? This might be an issue that gets fixed when the roles are fixed
  • Trapping focus - We trap focus for the keyboard user, which is awesome, but we need to trap focus for the screen reader user as well. (this is a change that we should make for all similar components, though)

These are all enhancements that can be made, but it's already pretty good accessibility wise. Great work, Katie!!

Copy link
Member

mcarrano left a comment

Looks great, @kmcfaul . I will mark 'UX Approved'.

@mcarrano mcarrano added ux approved and removed ux review labels Dec 12, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Dec 13, 2019

@kmcfaul I'm not seeing the external icon in the menus now. It has to do with the difference between the SVG icons used in react and the font awesome icons in core. We can make an update in core so that the icon works with your existing code, but it will require a fix after our code freeze.

Personally I think applying the class to the <span> like you had it is a better approach, as it allows for more flexible spacing and works with whatever kind of icon or image we want to use in it. Then we can update the examples in core to match. The other icons in the menu are all placed inside of an element that handles the spacing, too.

Otherwise LGTM! And when you pull in the latest version of core, the spacing of the elements that are directly below the search box should be fixed, too.

Titani
@tlabaj tlabaj dismissed stale reviews from mcarrano, jessiehuff, and themself via d730c10 Dec 16, 2019
@mcoker
mcoker approved these changes Dec 16, 2019
Copy link
Contributor

mcoker left a comment

LGTM! Thanks! 🏆

@tlabaj
tlabaj approved these changes Dec 16, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@dlabaj
dlabaj approved these changes Dec 16, 2019
@dlabaj dlabaj merged commit ce08747 into patternfly:master Dec 16, 2019
9 checks passed
9 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_a11y_pf4 Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Dec 16, 2019

Your changes have been released in:

  • @patternfly/react-catalog-view-extension@1.1.54
  • @patternfly/react-charts@5.2.6
  • @patternfly/react-core@3.128.0
  • @patternfly/react-docs@4.16.65
  • @patternfly/react-inline-edit-extension@2.14.14
  • demo-app-ts@3.15.0
  • @patternfly/react-integration@3.15.0
  • @patternfly/react-styled-system@3.7.14
  • @patternfly/react-styles@3.6.14
  • @patternfly/react-table@2.24.58
  • @patternfly/react-tokens@2.7.13
  • @patternfly/react-topology@2.11.42
  • @patternfly/react-virtualized-extension@1.3.55
  • @patternfly/react-icons@3.14.26

Thanks for your contribution! 🎉

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