Skip to content

fix: library dropdowns behavior #2210

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 44 additions & 1 deletion src/library-authoring/LibraryAuthoringPage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,49 @@ describe('<LibraryAuthoringPage />', () => {
});
});

it('can clear a single filter type', async () => {
await renderLibraryPage();

// Ensure the search endpoint is called
await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(1, searchEndpoint, 'post'); });
const filterButton = screen.getByRole('button', { name: /type/i });
fireEvent.click(filterButton);

// Validate click on Problem type
const problemMenu = screen.getAllByText('Problem')[0];
expect(problemMenu).toBeInTheDocument();
fireEvent.click(problemMenu);
await waitFor(() => {
expect(fetchMock).toHaveBeenLastCalledWith(searchEndpoint, {
body: expect.stringContaining('block_type = problem'),
method: 'POST',
headers: expect.anything(),
});
});

fireEvent.click(problemMenu);
await waitFor(() => {
expect(fetchMock).toHaveBeenLastCalledWith(searchEndpoint, {
body: expect.not.stringContaining('block_type = problem'),
method: 'POST',
headers: expect.anything(),
});
});

// Validate clear filters
fireEvent.click(problemMenu);

const clearFitlerButton = screen.getByText('Clear Filter');
fireEvent.click(clearFitlerButton);
await waitFor(() => {
expect(fetchMock).toHaveBeenLastCalledWith(searchEndpoint, {
body: expect.not.stringContaining('block_type = problem'),
method: 'POST',
headers: expect.anything(),
});
});
});

it('has an empty type filter when there are no results', async () => {
fetchMock.post(searchEndpoint, returnEmptyResult, { overwriteRoutes: true });
await renderLibraryPage();
Expand Down Expand Up @@ -1025,7 +1068,7 @@ describe('<LibraryAuthoringPage />', () => {
fireEvent.click(screen.getByRole('button', { name: /type/i }));
fireEvent.click(screen.getByRole('checkbox', { name: /text/i }));
// Escape to close the Types filter drop-down and re-enable the tabs
fireEvent.keyDown(screen.getByRole('button', { name: /type/i }), { key: 'Escape' });
fireEvent.keyDown(screen.getByRole('button', { name: /type:/i }), { key: 'Escape' });

// Navigate to the collections tab
fireEvent.click(await screen.findByRole('tab', { name: 'Collections' }));
Expand Down
23 changes: 17 additions & 6 deletions src/library-authoring/components/BaseCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,23 @@ const BaseCard = ({
title={
<Icon src={itemIcon} className="library-item-header-icon" />
}
actions={
// Wrap the actions in a div to prevent the card from being clicked when the actions are clicked
/* eslint-disable-next-line jsx-a11y/click-events-have-key-events,
jsx-a11y/no-static-element-interactions */
<div onClick={(e) => e.stopPropagation()}>{actions}</div>
}
actions={(
// eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions
<div
onClick={(e) => {
// Wrap the actions in a div to prevent the card from being clicked when the actions are clicked.
const target = e.target as HTMLElement;
const isDropdownToggle = target.closest('[data-toggle="dropdown"], .dropdown-toggle, [id*="dropdown"]');

// But allow dropdown coordination events to bubble up for proper dropdown behavior.
if (!isDropdownToggle) {
e.stopPropagation();
}
}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanielVZ96 Could you apply the same to the section/subsection child preview cards:

image

And to the Unit child preview cards:

image

>
{actions}
</div>
)}
/>
<Card.Body className="w-100">
<Card.Section>
Expand Down
2 changes: 1 addition & 1 deletion src/search-manager/FilterByPublished.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ const FilterByPublished = ({
onChange={() => { toggleFilterMode(filter); }}
>
<div>
{filterLabels[filter]}
{filterLabels[filter]}{' '}
<Badge variant="light" pill>{publishStatus[filter] ?? 0}</Badge>
Comment on lines +65 to 66
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, our translated filter labels should be formatted text that includes a variable for this count badge, so that RTL languages can opt to put the count on the left side of the text label. But since that's out of scope for this change it's ok to do as a follow-up PR.

CC @ChrisChV

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice find

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, this spacing can be handled with CSS.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanielVZ96 I agree with Jill, could you use CSS for the spacing ({' '})?

Actually, our translated filter labels should be formatted text that includes a variable for this count badge, so that RTL languages can opt to put the count on the left side of the text label. But since that's out of scope for this change it's ok to do as a follow-up PR.

I will create an issue for this

</div>
</MenuItem>
Expand Down
67 changes: 26 additions & 41 deletions src/search-manager/SearchFilterWidget.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
import React from 'react';
import { ArrowDropDown } from '@openedx/paragon/icons';
import {
Badge,
Button,
ModalPopup,
useToggle,
Dropdown,
} from '@openedx/paragon';
import { useIntl } from '@edx/frontend-platform/i18n';

Expand All @@ -27,60 +25,47 @@ const SearchFilterWidget: React.FC<{
children: React.ReactNode;
clearFilter: () => void,
icon: React.ComponentType;
}> = ({ appliedFilters, ...props }) => {
}> = ({
appliedFilters, children, clearFilter, ...props
}) => {
const intl = useIntl();
const [isOpen, open, close] = useToggle(false);
const [target, setTarget] = React.useState<HTMLButtonElement | null>(null);

const clearAndClose = React.useCallback(() => {
props.clearFilter();
close();
}, [props.clearFilter]);
clearFilter();
}, [clearFilter]);

return (
<>
<div className="d-flex mr-3">
<Button
ref={setTarget}
<div className="d-flex mr-3">
<Dropdown id={`search-filter-dropdown-${String(props.label).toLowerCase().replace(/\s+/g, '-')}`}>
<Dropdown.Toggle
variant={appliedFilters.length ? 'light' : 'outline-primary'}
size="sm"
onClick={open}
iconBefore={props.icon}
iconAfter={ArrowDropDown}
>
{props.label}
{appliedFilters.length >= 1 ? <>: {appliedFilters[0].label}</> : null}
{appliedFilters.length > 1 ? <>,&nbsp;<Badge variant="secondary">+{appliedFilters.length - 1}</Badge></> : null}
</Button>
</div>
<ModalPopup
positionRef={target}
isOpen={isOpen}
onClose={close}
>
<div
</Dropdown.Toggle>
<Dropdown.Menu
className="bg-white rounded shadow"
style={{ textAlign: 'start' }}
>
{props.children}
{children}

{
!!appliedFilters.length
&& (
<div className="d-flex justify-content-end">
<Button
onClick={clearAndClose}
variant="link"
className="text-info-500 text-decoration-none clear-filter-button"
>
{ intl.formatMessage(messages.clearFilter) }
</Button>
</div>
)
}
</div>
</ModalPopup>
</>
{!!appliedFilters.length && (
<div className="d-flex justify-content-end">
<Button
onClick={clearAndClose}
variant="link"
className="text-info-500 text-decoration-none clear-filter-button"
>
{ intl.formatMessage(messages.clearFilter) }
</Button>
</div>
)}
</Dropdown.Menu>
</Dropdown>
</div>
);
};

Expand Down
7 changes: 5 additions & 2 deletions src/search-modal/SearchUI.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -473,14 +473,17 @@ describe('<SearchUI />', () => {
});

it('can do a keyword search of the tag options', async () => {
const { getByRole, getByLabelText, queryByLabelText } = rendered;
const {
getByRole, getByLabelText, queryByLabelText, getByPlaceholderText,
} = rendered;
// Now open the filters menu:
fireEvent.click(getByRole('button', { name: 'Tags' }), {});
// The dropdown menu in this case doesn't have a role; let's just assume it's displayed.
const expandButtonLabel = /Expand to show child tags of "ESDC Skills and Competencies"/i;
await waitFor(() => { expect(getByLabelText(expandButtonLabel)).toBeInTheDocument(); });

const input = getByRole('searchbox');
// can't use getByRole because there are multiple searchboxes
const input = getByPlaceholderText(/search tags/i);
fireEvent.change(input, { target: { value: 'Lightcast' } });

await waitFor(() => { expect(queryByLabelText(/^ESDC Skills and Competencies/i)).toBeNull(); });
Expand Down