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.

Sign up for GitHub

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

[EuiCollapsibleNavBeta] Final collapsed/docked icon & popover behavior #7034

Merged
merged 14 commits into from
Aug 7, 2023
Merged

[EuiCollapsibleNavBeta] Final collapsed/docked icon & popover behavior #7034

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5dff814
[misc] Clean up `EuiCollapsibleNavItem` className rendering
cee-chen Aug 3, 2023
b540a06
[misc] Clean up `EuiCollapsibleNavLink` handling of `linkProps`
cee-chen Aug 4, 2023
5b7869b
Set up `EuiCollapsibleNavContext`
cee-chen Aug 3, 2023
b19eddc
Set up `EuiCollapsedNavButton` component
cee-chen Aug 3, 2023
5e22be8
Set up `EuiCollapsedNavPopover` component
cee-chen Aug 4, 2023
1cff317
Set up `EuiCollapsedNavItem` component
cee-chen Aug 4, 2023
0c3cb8d
Final logic for switching between `EuiCollapsedNavItem` and `EuiColla…
cee-chen Aug 4, 2023
965b476
[Designer feedback] Remove button hover effect
cee-chen Aug 4, 2023
0fce843
[polish] Fix tooltips overlapping popovers
cee-chen Aug 4, 2023
17b8c62
[polish] Fix bottom-most button not having a right-anchored popover
cee-chen Aug 4, 2023
f9838a3
[polish] Hide vertical scrollbar in docked/collapsed icon mode
cee-chen Aug 4, 2023
d6c7f7e
[Designer feedback] Add slight shadow to nav
cee-chen Aug 4, 2023
a582ddc
[cleanup] Rename various responsive variables
cee-chen Aug 4, 2023
2a1ec67
[PR design feedback] Accordion arrows are rotated the wrong way :face…
cee-chen Aug 4, 2023
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
5 changes: 5 additions & 0 deletions ...onents/collapsible_nav_beta/collapsible_nav_item/collapsed/collapsed_nav_button.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,5 +45,10 @@ export const euiCollapsedNavItemTooltipStyles = ({ euiTheme }: UseEuiTheme) => {
right: css`
${logicalCSS('margin-right', `-${euiTheme.size.m}`)}
`,
// If the item has a popover and the popover is open, we don't want the
// tooltip to appear if so - the popover already renders the item title
hidden: css`
display: none;
`,
Comment on lines +48 to +52
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this extra logic/CSS, you get this silliness:

tooltip

You can still overlap the tooltips of other nav items onto open popovers, but at least that doesn't repeat information.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch!

};
};
16 changes: 15 additions & 1 deletion ...ponents/collapsible_nav_beta/collapsible_nav_item/collapsed/collapsed_nav_button.test.tsx
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just want to note - I know this test (and dev tools) triggers this warning repeatedly:

I'm opting to skip it for now and tackle it in a separate accessibility PR/pass. The main reason it's actually not a major a11y issue is because of the wrapping EuiToolTip, which sets an aria-describedby on the button icon.

So the title will still get read out, albeit less quickly than an aria-label would. But if I try to set an aria-label now, what happens is the title then gets repeated twice due to the tooltip describedby, which TBH feels worse. So I'd rather try to fix the issue more completely, hence the separate follow-up work.

Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

import React from 'react';
import { fireEvent } from '@testing-library/react';
import { fireEvent, waitFor } from '@testing-library/react';
import { render, waitForEuiToolTipVisible } from '../../../../test/rtl';
import { shouldRenderCustomStyles } from '../../../../test/internal';
import { requiredProps } from '../../../../test';
Expand Down Expand Up @@ -35,4 +35,18 @@ describe('EuiCollapsedNavButton', () => {
);
expect(container).toMatchSnapshot();
});

// This is primarily used by EuiCollapsedNavPopover to hide the tooltip
// when the popover is open (otherwise it overlays the popover)
it('allows hiding the tooltip', async () => {
const { getByTestSubject } = render(
<EuiCollapsedNavButton title="Nav item" href="#" hideToolTip={true} />
);
fireEvent.mouseOver(getByTestSubject('euiCollapsedNavButton'));

await waitFor(() => {
const tooltip = document.querySelector('.euiToolTipPopover');
expect(tooltip).toHaveStyleRule('display', 'none');
});
});
});
6 changes: 5 additions & 1 deletion src/components/collapsible_nav_beta/collapsible_nav_item/collapsed/collapsed_nav_button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,17 @@ import {
} from './collapsed_nav_button.styles';

export const EuiCollapsedNavButton: FunctionComponent<
EuiCollapsibleNavItemProps
EuiCollapsibleNavItemProps & {
hideToolTip?: boolean;
}
> = ({
href, // eslint-disable-line local/href-with-rel
title,
icon,
iconProps,
isSelected,
onClick,
hideToolTip,
linkProps,
// Extracted to avoid spreading to ...rest
accordionProps,
Expand All @@ -58,6 +61,7 @@ export const EuiCollapsedNavButton: FunctionComponent<
const tooltipCssStyles = [
tooltipStyles.euiCollapsedNavItemTooltip,
tooltipStyles[side],
hideToolTip && tooltipStyles.hidden,
];

return (
Expand Down
1 change: 1 addition & 0 deletions src/components/collapsible_nav_beta/collapsible_nav_item/collapsed/collapsed_nav_popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ export const EuiCollapsedNavPopover: FunctionComponent<
iconProps={iconProps}
isSelected={isSelected}
onClick={togglePopover}
hideToolTip={isPopoverOpen}
// Note: do not pass `linkProps` to buttons that toggle popovers
/>
}
Expand Down