docs(Dropdown): add composable dropdown demo, convert composable menu demos to typescript #6807
Conversation
There was a problem hiding this comment.
This looks good to me from an a11y standpoint. It does remind me of pre-existing issues that Dropdown has though such as tooltips on dropdown items and the Append menu document body issue.
|
|
||
| export const ComposableActionsMenu: React.FunctionComponent = () => { | ||
| const [isOpen, setIsOpen] = React.useState(false); | ||
| const [selectedItems, setSelectedItems] = React.useState([]); |
There was a problem hiding this comment.
We can add a type to the useState of number[] here.
And based on a separate discussion we had, boolean to the one above it.
There was a problem hiding this comment.
I agree with adding a type to React.useState([]). Adding boolean to React.useState(false) seems redundant, considering this type is inferred
| import SearchIcon from '@patternfly/react-icons/dist/esm/icons/search-icon'; | ||
|
|
||
| export const ComposableContextSelector: React.FunctionComponent = () => { | ||
| const items: any[] = [ |
There was a problem hiding this comment.
Is there no type associated with these or are they meant to be any?
nicolethoen
force-pushed
the
composable_dropdown_demos
branch
from
ac6a824
to
6a6cb39
Compare
| const [activeMenu, setActiveMenu] = React.useState<string>('rootMenu'); | ||
| const [menuDrilledIn, setMenuDrilledIn] = React.useState<string[]>([]); | ||
| const [drilldownPath, setDrilldownPath] = React.useState<string[]>([]); | ||
| const [menuHeights, setMenuHeights] = React.useState<any>({}); |
There was a problem hiding this comment.
Nit: wouldn't this any be better typed with an index signature like { [id: string]: number }?
|
I have a pending question to @mattnolting I’m noticing that the drilldown menu example here: https://patternfly-react-pr-6807.surge.sh/demos/composable-menu#composable-drilldown-menu has an extra scroll bar when I begin drilling down. We are setting the menu height correctly (i think) but is there some extra padding or something that isn’t being accounted for? |
nicolethoen
force-pushed
the
composable_dropdown_demos
branch
from
f648ce2
to
8db64a8
Compare
|
This is ready to go in (i think) The issue I noted above in an existing bug with the drilldown menu - I noted it here: #6842 |
|
@mcarrano I opened an issue to build the split button that is required to implement the other dropdown variants. I am not sure if it is reasonable at this point to assume every single example will be exactly re-implemented composably... I think collectively, these demos are telling a story of how versatile our composable approach is. and people can open issues if they want to see more examples? |
|
@nicolethoen Yes, I agree. I don't think it's necessary to re-implement every example unless we are specifically asked for it. |
| return ( | ||
| <MenuToggle | ||
| ref={toggleRef} | ||
| icon={<Avatar src={avatarImg} alt="avatar" />} |
There was a problem hiding this comment.
It looks like we use .pf-c-menu-toggle__image in core for this versus .pf-c-menu-toggle__icon, though I don't know that we need both classes - curious if we could just use a single class for both.
Some other thoughts:
.pf-c-menu-toggle__imageshouldn't be used right now since there is a bug in the spacing - the right margin for the image isn't being applied, so the image will touch the adjacent text.- the dropdown image toggle image element has some additional CSS. It applies a top/bottom margin, too, and uses
inline-flexandflex-shrink: 0;. I'm not sure about the top/bottom margin, but I think the flex properties are good adds.
- the image isn't really aligned. we could apply
line-height: 1; align-self: centerto the__image/iconelement. This is what it looks like in the PR.
I think we could just apply all of this to a single element that holds an icon or image. wdyt @mattnolting?
There was a problem hiding this comment.
@mcoker I actually added the flex-shrink prop in 4617 as the image was distorted when width became constrained. I agree, we only need one container element and it should be __image IMO.
I'm not sure about the top/bottom margin, but I think the flex properties are good adds.
We should nix top/bottom margin
the image isn't really aligned. we could apply line-height: 1; align-self: center to the __image/icon element. This is what it looks like in the PR.
👍 👍 👍
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
Closes #6016
Adds the Composable Dropdown Variants demo to the Composable Menu demos