(feat) O3-4335: Enable multiple filters on the appointments app dashboard #1449
Conversation
|
Hi @Samstar10 - this looks good to me based on the screenshot. Do the numbers update in the metrics cards? E.g. 'Scheduled appointments'? |
Yes they do @paulsonder |
| {items.map((item) => ( | ||
| <React.Fragment key={item.uuid}> | ||
| <MenuItemSelectable | ||
| key={item.uuid} |
There was a problem hiding this comment.
| key={item.uuid} |
The parent React Fragment already has a key, so we can rid of this duplicate one.
| aria-label={t('filterByServiceType', 'Filter by service type')} | ||
| id="serviceMenu" | ||
| > | ||
| {items.map((item) => ( |
There was a problem hiding this comment.
| {items.map((item) => ( | |
| {items.map((item, i) => ( |
| onChange={() => handleMenuItemChange(item.uuid)} | ||
| onClick={(e) => e.stopPropagation()} | ||
| /> | ||
| <MenuItemDivider /> |
There was a problem hiding this comment.
| <MenuItemDivider /> | |
| {i < items.length - 1 && <MenuItemDivider />} |
So we don't render a divider after the last menuitem.
| useEffect(() => { | ||
| onChange?.(''); | ||
| }, []) |
There was a problem hiding this comment.
| useEffect(() => { | |
| onChange?.(''); | |
| }, []) | |
| useEffect(() => { | |
| onChange?.([]); | |
| }, [onChange]) |
Shouldn't this be initialized with an empty array? Also, we need to add the onChange function to the effect's dependency array.
There was a problem hiding this comment.
@denniskigen if we initialise the onChange function using an empty array the table becomes reset to an empty state rather than showing all categories on initialisation as is intended.
| onChange?: (evt) => void; | ||
| } | ||
|
|
||
| const AppointmentsHeader: React.FC<AppointmentHeaderProps> = ({ title, appointmentServiceType, onChange }) => { | ||
| const { t } = useTranslation(); | ||
| const { selectedDate, setSelectedDate } = useContext(SelectedDateContext); | ||
| const { serviceTypes } = useAppointmentServices(); | ||
|
|
||
| const items = [{ name: 'All', uuid: '' }, ...serviceTypes]; |
There was a problem hiding this comment.
Isn't this All option redundant now that we're in a multi-select context? My assumption is that in a multi-select, "no selection" effectively means "show all". Having an "All" option creates ambiguous states: what happens when "All" and an individual service are selected?
I think it's more intuitive to allow the user to just select the specific services they want to filter by.
There was a problem hiding this comment.
@denniskigen when the "All" option is selected, it resets all other filters selected and also when a filter is selected the "All" is automatically unselected. This is convenient for when a user selects multiple filters and wants to go back to seeing all the categories, they don't have to go back and unselect all the selected categories but instead they can just click "All" and have everything reset. So when a filter is selected, the All is automatically unselected and vice versa.
| const items = [{ name: 'All', uuid: '' }, ...serviceTypes]; | ||
| const [selectedItems, setSelectedItems] = useState([items[0]]); | ||
|
|
||
| const handleMenuItemChange = (itemUuid: string) => { |
There was a problem hiding this comment.
| const handleMenuItemChange = (itemUuid: string) => { | |
| const handleMenuItemChange = useCallback(itemUuid: string) => { |
Let's wrap this in a useCallback hook.
|
@paulsonder @ciaranduffy I'm not entirely sure that the MenuButton is the correct Carbon component to use for this table filter. On the one hand, it makes sense to use it if the goal is to hide extra actions in limited screen space:
But it doesn't make sense if the goal is to display a list of filters:
The MultiSelect component is better suited for displaying a list of filters, but its not ideal for situations where space is limited. So, I'm unsure which component is the best fit. To add another consideration, this problem has been solved in a different way in the service queues page where a list of filters are displayed in the table header.
One specific aspect I like about the approach there is the proximity of the filters to the table for the following reasons:
Do you think we should consider adopting this approach for the appointments page? |
@denniskigen should I wait for response on this before I push the requested changes? |
|
Thanks for your patience while we got around to this @Samstar10 and thank you @denniskigen for catching this point. You're absolutely correct to say that the MenuButton isn't the correct component here and the component you've pointed to in the Service Queues would in fact be more suitable. That component is https://carbondesignsystem.com/components/dropdown/usage/ and it's the Hope that's helpful and helps you move this forward |
Hello @ciaranduffy, the dropdown component does not allow selection of multiple items and wouldn't be able to meet the feature requirements. |
|
@Samstar10 wouldn't you just use the multi-select variant of the same component then? I think this pattern could be improved though. Carbon's multi-select component doesn't do a great job of showing you what filters are applied, only that two (or more) filters are applied, but that's something that could be improved in a future iteration, I think. |
Alright @ciaranduffy, that sounds goods to me. I'll get it done. Thank you. |
Requirements
Summary
This PR introduces a new feature that allows a user to filter appointments by multiple service types at once in the Appointments app dashboard.
Screenshots
Related Issue
https://openmrs.atlassian.net/browse/O3-4335
Other