patternfly-react icon indicating copy to clipboard operation
patternfly-react copied to clipboard

Adds content to dropdown react examples.

Open edonehoo opened this issue 3 years ago • 10 comments
trafficstars

Makes progress on patternfly/patternfly-org/issues/2990

edonehoo avatar Aug 02 '22 13:08 edonehoo

Preview: https://patternfly-react-pr-7784.surge.sh

A11y report: https://patternfly-react-pr-7784-a11y.surge.sh

patternfly-build avatar Aug 02 '22 14:08 patternfly-build

I believe the workspaces has a quirk where you need to refresh the page to see the props and css vars tables at the bottom of the docs. I think it's been that way for a bit - but it's not happening on the live site - only the workspace. May be worth some investigation but probably not a result of something done in this PR

nicolethoen avatar Aug 19 '22 12:08 nicolethoen

This looks great! Just left a comment about a typo, and agree with Matt around adding more context on when to use the split buttons!

mmenestr avatar Aug 19 '22 19:08 mmenestr

I still want to move things around and reword sections, but wanted to go ahead and push updates.

Question -- is there a reason to have both the "with kebab" and "icon only" examples? I know they're slightly different, but I'm struggling to write content that justifies them being separate.

edonehoo avatar Aug 31 '22 20:08 edonehoo

@edonehoo I agree that the "with Kabob" and "with Icon" examples are functionally equivalent. But looking at the code seems they are slightly different? Seems like the Kabob is a unique thing vs just putting an icon there. @mcoker or @tlabaj can you help clarify.

If they are to be unique, then I might say something like "A dropdown as a kabob menu is used to hold a list of actions when you have limited space." and for the icon example, "Any icon can be used as a dropdown toggle as a substitute for a label. Only use this when the icon is likely to convey meaning about the contents of the menu." Or something like that...

mcarrano avatar Aug 31 '22 21:08 mcarrano

@mcarrano @edonehoo looks like we have 2 ways of creating the plain/icon dropdown toggle.

  1. Using a unique dropdown toggle called <KebabToggle> that gives you the kebab/vertical ellipsis icon (you can't change it) and removes the down arrow/caret icon on the right of the dropdown toggle for you. The "with kebab" example shows how to use <KebabToggle> with the dropdown.

  2. Using our standard <DropdownToggle> that's used for most dropdown menus (toggles with text and whatever). The "Icon only" example shows how you can pass an icon (instead of text) to <DropdownToggle>, and specify the toggleIndicator={null} prop to manually remove the down arrow/caret icon to create a plain/icon toggle. You can use whatever icon you want there.

mcoker avatar Aug 31 '22 23:08 mcoker

A few (hopefully) final questions -- thanks for all the context so far!

  1. Is it correct to say "plain text toggle", though the props are on the Dropdown component, rather than DropdownToggle? It seems like this example only styles the toggle, but I'm curious why this works applying these props to Dropdown rather than DropdownToggle? This may just be some behind the scenes PF stuff, but I mainly wanted to be sure the heading was appropriate!
  2. Would it make sense to change the "With initial focus" heading to "With autofocus" since that corresponds to the actual property a little better? Or would adding more/different context to the heading help it make more sense? For some reason, it doesn't feel like a perfect heading, but I'm curious for other's thoughts.
  3. I don't have much understanding about the basic panel example. Could someone offer a little more context on why this would be used, or if I should give any additional direction on how to add a custom menu/basic panel?

edonehoo avatar Sep 07 '22 21:09 edonehoo

My two cents on the above comments:

Is it correct to say "plain text toggle", though the props are on the Dropdown component, rather than DropdownToggle? It seems like this example only styles the toggle, but I'm curious why this works applying these props to Dropdown rather than DropdownToggle? This may just be some behind the scenes PF stuff, but I mainly wanted to be sure the heading was appropriate!

I'd say it's correct despite the props being applied to the Dropdown component, since only the toggle is being styled. It essentially is some behind the scenes stuff where even though the isText and isPlain props are being passed into Dropdown, they're being applied to DropdownToggle.

Would it make sense to change the "With initial focus" heading to "With autofocus" since that corresponds to the actual property a little better? Or would adding more/different context to the heading help it make more sense? For some reason, it doesn't feel like a perfect heading, but I'm curious for other's thoughts.

I think it would make sense to update the example title to "With autofocus", or maybe "Autofocus on expand", "Autofocus dropdown item" or something.

I don't have much understanding about the basic panel example. Could someone offer a little more context on why this would be used, or if I should give any additional direction on how to add a custom menu/basic panel?

That's a good question. Currently our advanced search input example uses Panel components behind the scenes when the menu is open. Because of how we describe a dropdown, though ("A dropdown presents a menu of actions or links in a constrained space that will trigger a process or navigate to a new location."), I do wonder whether a dropdown with panels should be an example since maybe other components may be better suited for "custom menus" in a dropdown-like menu.

If a basic panel example is kept, then the example could probably be updated to utilize Panel components or otherwise showcase a possible implementation.

thatblindgeye avatar Sep 13 '22 13:09 thatblindgeye

I think it would make sense to update the example title to "With autofocus", or maybe "Autofocus on expand", "Autofocus dropdown item" or something.

+1 "Autofocus on expand"

nicolethoen avatar Sep 13 '22 20:09 nicolethoen

3. I don't have much understanding about the basic panel example. Could someone offer a little more context on why this would be used, or if I should give any additional direction on how to add a custom menu/basic panel?

This dropdown is intended for use when someone wants custom content (i.e. not use dropdown items) in the dropdown menu. In that case, they'd be responsible to style the contents of the custom content in the panel and for focus management within the panel as well.

as @thatblindgeye noted, it is a little confusing that we use the word panel here (though it was used here first), since we now have a Panel component, which this is not meant to reference. It may be more accurate to describe it as custom content rather than dropdown items in the dropdown menu

nicolethoen avatar Sep 13 '22 20:09 nicolethoen

@edonehoo This all looks great except for this one example: https://patternfly-react-pr-7784.surge.sh/components/dropdown#split-button-indeterminate-state. I'm not exactly sure why the example is titled as it is, but seems like this is just a standard split button with a checkbox. The fact that it has a mixed (or indeterminate?) state (-) is just to indicate that onlt some elements are selected (i.e. in a bulk selector ref: https://www.patternfly.org/v4/guidelines/bulk-selection) But it doesn't seem to make much sense out of context. @tlabaj can you elaborate on what we are trying to illustrate with this example? Should we rename it?

@mcarrano I am not sure how that example came about, It was probably added to demo the functionality.
I think the example can be remove. I would however add back the basic "Split button" example that was removed in this PR. The code is in DropdownSplitButton.tsx. I will leave a comment inline as well.

tlabaj avatar Sep 26 '22 18:09 tlabaj

Your changes have been released in:

Thanks for your contribution! :tada:

patternfly-build avatar Sep 30 '22 16:09 patternfly-build