Skip to content
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

fix(listbox): show selection toolbar when listbox opens in popover #1204

Merged
merged 4 commits into from
Apr 3, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
34 changes: 23 additions & 11 deletions apis/nucleus/src/components/listbox/ListBoxInline.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ const Title = styled(Typography)(({ theme }) => ({
fontFamily: theme.listBox?.title?.main?.fontFamily,
fontWeight: theme.listBox?.title?.main?.fontWeight || 'bold',
}));

const isModal = ({ app, appSelections }) => app.isInModalSelection?.() ?? appSelections.isInModal();

function ListBoxInline({ options, layout }) {
Expand All @@ -85,6 +84,7 @@ function ListBoxInline({ options, layout }) {
scrollState = undefined,
renderedCallback,
toolbar = true,
isPopover = false,
} = options;

// Hook that will trigger update when used in useEffects.
Expand Down Expand Up @@ -140,8 +140,8 @@ function ListBoxInline({ options, layout }) {
isModal: isModalMode,
});

const showDetachedToolbarOnly = toolbar && (layout?.title === '' || layout?.showTitle === false);
const showToolbarWithTitle = toolbar && layout?.title !== '' && layout?.showTitle !== false;
const showDetachedToolbarOnly = toolbar && (layout?.title === '' || layout?.showTitle === false) && !isPopover;
const showToolbarWithTitle = (toolbar && layout?.title !== '' && layout?.showTitle !== false) || isPopover;

useEffect(() => {
const show = () => {
Expand All @@ -153,6 +153,14 @@ function ListBoxInline({ options, layout }) {
setShowSearch(false);
}
};
if (isPopover) {
if (!selections.isActive()) {
selections.begin('/qListObjectDef');
selections.on('activated', show);
selections.on('deactivated', hide);
}
setShowToolbar(isPopover);
}
Comment on lines +156 to +163
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe also add a check if selections exists. based on that the next if does maybe it is also needed here?

if (selections) {
if (!selections.isModal()) {
selections.on('activated', show);
Expand All @@ -167,7 +175,7 @@ function ListBoxInline({ options, layout }) {
selections.removeListener('deactivated', hide);
}
};
}, [selections]);
}, [selections, isPopover]);

useEffect(() => {
if (!searchContainer || !searchContainer.current) {
Expand Down Expand Up @@ -219,9 +227,9 @@ function ListBoxInline({ options, layout }) {
}
};

const getActionToolbarProps = (isPopover) =>
const getActionToolbarProps = (isDetached) =>
getListboxActionProps({
isPopover,
isDetached: isPopover ? false : isDetached,
showToolbar,
containerRef,
isLocked,
Expand All @@ -236,6 +244,7 @@ function ListBoxInline({ options, layout }) {
const iconsWidth = (showSearchOrLockIcon ? BUTTON_ICON_WIDTH : 0) + (isDrillDown ? ICON_WIDTH + ICON_PADDING : 0); // Drill-down icon needs padding right so there is space between the icon and the title
const headerPaddingLeft = CELL_PADDING_LEFT - (showSearchOrLockIcon ? ICON_PADDING : 0);
const headerPaddingRight = isRtl ? CELL_PADDING_LEFT - (showIcons ? ICON_PADDING : 0) : 0;
const { isDetached, reasonDetached } = showToolbarDetached({ containerRef, titleRef, iconsWidth });

// Add a container padding for grid mode to harmonize with the grid item margins (should sum to 8px).
const isGridMode = layoutOptions?.dataLayout === 'grid';
Expand Down Expand Up @@ -318,18 +327,21 @@ function ListBoxInline({ options, layout }) {
)}
<Grid item sx={{ justifyContent: isRtl ? 'flex-end' : 'flex-start' }} className={classes.listBoxHeader}>
{showTitle && (
<Title variant="h6" noWrap ref={titleRef} title={layout.title}>
<Title
variant="h6"
sx={{ width: isPopover && reasonDetached === 'noSpace' ? '60px' : undefined }}
noWrap
ref={titleRef}
title={layout.title}
>
{layout.title}
</Title>
)}
</Grid>
</Grid>
<Grid item xs />
<Grid item>
<ActionsToolbar
direction={direction}
{...getActionToolbarProps(showToolbarDetached({ containerRef, titleRef, iconsWidth }))}
/>
<ActionsToolbar direction={direction} {...getActionToolbarProps(isDetached)} />
</Grid>
</Grid>
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ describe('<ListboxInline />', () => {
selections,
update: undefined,
fetchStart: 'fetchStart',
isPopover: false,
};

useRef.mockReturnValue({ current: 'current' });
Expand Down Expand Up @@ -260,6 +261,24 @@ describe('<ListboxInline />', () => {
});
});

test('should show toolbar when opened in a popover', async () => {
options.search = false;
options.isPopover = true;
await render();
const actionToolbars = renderer.root.findAllByType(ActionsToolbar);
expect(actionToolbars).toHaveLength(1);

const typographs = renderer.root.findAllByType(Typography);
expect(typographs).toHaveLength(1);

expect(ListBoxSearch.mock.calls[0][0]).toMatchObject({
visible: false,
});
expect(selections.on).toHaveBeenCalledTimes(2);
expect(selections.on.mock.calls[0][0]).toBe('activated');
expect(selections.on.mock.calls[1][0]).toBe('deactivated');
});

test('should remove correct listeners on unmount', async () => {
await render();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,27 @@ describe('show listbox toolbar detached', () => {
it('should return true if there is not enough space for toolbar', () => {
const containerRef = { current: { clientWidth: 100 } };
const titleRef = { current: { clientWidth: 60, scrollWidth: 80, offsetWidth: 81 } };
expect(showToolbarDetached({ containerRef, titleRef, iconsWidth })).toBe(true);
expect(showToolbarDetached({ containerRef, titleRef, iconsWidth })).toStrictEqual({
isDetached: true,
reasonDetached: 'noSpace',
});
});

it('should return true if title is truncated', () => {
const containerRef = { current: { clientWidth: 300 } };
const titleRef = { current: { clientWidth: 60, scrollWidth: 200, offsetWidth: 199 } };
expect(showToolbarDetached({ containerRef, titleRef, iconsWidth })).toBe(true);
expect(showToolbarDetached({ containerRef, titleRef, iconsWidth })).toStrictEqual({
isDetached: true,
reasonDetached: 'truncated',
});
});

it('should return false if there is enough space for title and toolbar', () => {
const containerRef = { current: { clientWidth: 300 } };
const titleRef = { current: { clientWidth: 60, scrollWidth: 200, offsetWidth: 201 } };
expect(showToolbarDetached({ containerRef, titleRef, iconsWidth })).toBe(false);
expect(showToolbarDetached({ containerRef, titleRef, iconsWidth })).toStrictEqual({
isDetached: false,
reasonDetached: '',
});
});
});
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
export default function getListboxActionProps({
isPopover,
isDetached,
showToolbar,
containerRef,
isLocked,
listboxSelectionToolbarItems,
selections,
}) {
return {
show: showToolbar && !isPopover,
show: showToolbar && !isDetached,
popover: {
show: showToolbar && isPopover,
show: showToolbar && isDetached,
anchorEl: containerRef.current,
},
more: {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
/* eslint-disable no-nested-ternary */
export default function showToolbarDetached({ containerRef, titleRef, iconsWidth }) {
const containerWidth = containerRef?.current?.clientWidth ?? 0;
const padding = 16;
const contentWidth = (titleRef?.current?.clientWidth ?? 0) + iconsWidth + padding;
const actionToolbarWidth = 128;
const notSufficientSpace = containerWidth < contentWidth + actionToolbarWidth;
const isTruncated = titleRef?.current?.scrollWidth > titleRef?.current?.offsetWidth;
const isDetached = !!(notSufficientSpace | isTruncated);
const reasonDetached = isDetached ? (notSufficientSpace ? 'noSpace' : 'truncated') : '';

return !!(notSufficientSpace | isTruncated);
return { isDetached, reasonDetached };
}