WAITP-1211: Entity Dropdown Menu #4672
Conversation
tcaiger
changed the base branch from
epic-frontend-rewrite
to
waitp-1212-breadcrumbs
tcaiger
changed the base branch from
waitp-1212-breadcrumbs
to
epic-frontend-rewrite
| }: { | ||
| projectCode: string; | ||
| children: EntityWithChildren[]; | ||
| isLoading: boolean; |
There was a problem hiding this comment.
Possibly we could add this as an interface above the component, to keep with our current pattern?
| const [isExpanded, setIsExpanded] = useState(false); | ||
| const { data, isLoading } = useEntities(projectCode!, entity.code!, { enabled: isExpanded }); | ||
|
|
||
| const onExpand = () => { |
There was a problem hiding this comment.
This is really pedantic, but I would maybe rename this to be toggleExpanded to make it more representative of its function
| /* | ||
| Pre-populate the next layer of the menu with children that came from the previous layer of entity | ||
| data then replace them with the children from the API response when it arrives | ||
| */ |
There was a problem hiding this comment.
Nice comments, really helpful
| <MenuLink to={link}> | ||
| {entity.name} {entity.type === 'facility' && <HospitalIcon />} | ||
| </MenuLink> | ||
| <IconButton onClick={onExpand} disabled={parentIsLoading || !nextChildren}> |
There was a problem hiding this comment.
What do you think of only having the button when there are nextChildren? From an accessibility perspective this would be preferred I think, so that there isn't a button announced to the user that doesn't do anything
There was a problem hiding this comment.
I thought about this and player around with it a bit. The button is really helpful to keep the spacing from glitching when the Icon button appears. The button is semantically disabled which helps.
| )} | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
It might be worth adding aria attributes to the menu, with aria-expanded on the menu as true/false. Also adding an aria-label to the expand button to explain what the button does would be good too
| import { ClickAwayListener } from '@material-ui/core'; | ||
| import { SearchBar } from './SearchBar'; | ||
| import { EntityMenu } from './EntityMenu'; | ||
| import { useParams } from 'react-router-dom'; |
There was a problem hiding this comment.
I think the linter will probably complain that this import isn't above local imports
| }; | ||
|
|
||
| return ( | ||
| <SearchInput value={value} onChange={onChangeInputValue} onFocus={() => onFocusChange(true)} /> |
There was a problem hiding this comment.
Not sure, but would it be useful to have an onBlur event listener here too?
There was a problem hiding this comment.
I initially had the onBlur event closing the entity menu but then I realised that the menu kept closing whenever I clicked on it and realised the blur was the problem so I don't think we want the blur for that interaction. Is there any thing else that you think should happen when you blur?
| <IconButton | ||
| onClick={toggleExpanded} | ||
| disabled={parentIsLoading || !nextChildren} | ||
| aria-controls="entity-expand-button" |
There was a problem hiding this comment.
Nice, thank you! Can we just add an aria-label of something like toggle menu for .... ?
| children={nextChildren} | ||
| projectCode={projectCode} | ||
| isLoading={isLoading} | ||
| aria-expanded={isExpanded} |
There was a problem hiding this comment.
Just checking - does this get carried down to the menu element?
There was a problem hiding this comment.
ah very good point. I have moved it to the List element and it's always true there since the menu list only shows when expanded.
* add breadcrumbs component * set up entity search * recursive menu * fix types * styles * remove breadcrumbs * Update EntityMenu.tsx * pre load entity data * get menu for project entity * fixes * Update EntityMenu.tsx
* add breadcrumbs component * set up entity search * recursive menu * fix types * styles * remove breadcrumbs * Update EntityMenu.tsx * pre load entity data * get menu for project entity * fixes * Update EntityMenu.tsx
Issue #: WAITP-1211: Entity Search
Changes:
Screenshots: