@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-React preview: https://patternfly-react-pr-3371.surge.sh

Codecov Report

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

@@            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

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.

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.

@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

Search finds applications in the same group

Search finds applications in multiple groups

**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?

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.

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".

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!!

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"?

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.

@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.

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?

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).

Updated with pr feedback and added cypress/TS demo

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

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

It looks great to me, @kmcfaul :)

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.

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

@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.

@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.

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! 🎉