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

Jump to bottom

Menu fixes #7017

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+300 −222
Open

Menu fixes #7017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0c3caa4
fix(menu, menu-item): ensure that menu-items all remain the same heig…
edleeks87 Oct 14, 2024
4c220a0
fix(menu-item): ensure that it does not render children when `submenu…
edleeks87 Oct 14, 2024
f262259
chore: some small tweaks by ed leeks tbs
edleeks87 Oct 18, 2024
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
4 changes: 2 additions & 2 deletions src/components/global-header/component.test-pw.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import NavigationBar from "../navigation-bar";
export const FullMenuExample = () => (
<>
<GlobalHeader>
<Menu menuType="black" display="flex" flex="1">
<Menu menuType="black" flex="1">
<MenuItem flex="1" submenu="Product Switcher">
<MenuItem href="#">Product A</MenuItem>
</MenuItem>
Expand All @@ -22,7 +22,7 @@ export const FullMenuExample = () => (
</Menu>
</GlobalHeader>
<NavigationBar position="fixed" orientation="top" offset="40px">
<Menu display="flex" flex="1">
<Menu flex="1">
<MenuItem flex="1">Menu Item One</MenuItem>
<MenuItem flex="0 0 auto" href="#">
Menu Item Two
Expand Down
2 changes: 1 addition & 1 deletion src/components/global-header/global-header-test.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export const MenuWithIconOnlyButtonsStory: StoryFn<
return (
<GlobalHeader logo={<img height={28} src={carbonLogo} alt="Carbon logo" />}>
<VerticalDivider h="100%" pt={1} pb={1} pr={0} pl={2} tint={100} />
<Menu menuType="black" display="flex" flex="1">
<Menu menuType="black" flex="1">
<MenuItem flex="1" submenu="Product Switcher">
<MenuItem href="#">Product A</MenuItem>
</MenuItem>
Expand Down
8 changes: 4 additions & 4 deletions src/components/global-header/global-header.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export const BasicMenu: Story = () => {

return (
<GlobalHeader logo={<Logo />}>
<Menu menuType="black" display="flex" flex="1">
<Menu menuType="black" flex="1">
<MenuItem flex="1" submenu="Product Switcher">
<MenuItem>Product A</MenuItem>
</MenuItem>
Expand Down Expand Up @@ -112,7 +112,7 @@ export const ResponsiveMenu: Story = () => {

return (
<GlobalHeader logo={<Logo />}>
<Menu menuType="black" display="flex" flex="1">
<Menu menuType="black" flex="1">
{fullscreenViewBreakPoint ? (
<>
<MenuItem
Expand Down Expand Up @@ -145,7 +145,7 @@ export const GlobalLocalNavBarLayout: Story = () => {
return (
<>
<GlobalHeader logo={<Logo />}>
<Menu menuType="black" display="flex" flex="1">
<Menu menuType="black" flex="1">
<MenuItem flex="1" submenu="Product Switcher">
<MenuItem href="#">Product A</MenuItem>
</MenuItem>
Expand All @@ -160,7 +160,7 @@ export const GlobalLocalNavBarLayout: Story = () => {
</Menu>
</GlobalHeader>
<NavigationBar position="fixed" orientation="top" offset="40px">
<Menu display="flex" flex="1">
<Menu flex="1">
<MenuItem href="#" flex="1">
Menu Item One
</MenuItem>
Expand Down
15 changes: 9 additions & 6 deletions src/components/menu/__internal__/submenu/submenu.style.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,16 @@ const StyledSubmenuWrapper = styled.div<StyledSubmenuWrapperProps>`
position: relative;
width: fit-content;
max-width: inherit;
height: inherit;
${({ isSubmenuOpen, theme }) =>
isSubmenuOpen &&
css`
z-index: ${theme.zIndex.popover};
`}
${({ inFullscreenView, menuType, asPassiveItem }) =>
inFullscreenView &&
${({ inFullscreenView, menuType, asPassiveItem }) => css`
${inFullscreenView &&
css`
width: 100%;
Expand All @@ -55,6 +56,11 @@ const StyledSubmenuWrapper = styled.div<StyledSubmenuWrapperProps>`
}
`}
`}
${!inFullscreenView &&
css`
display: flex;
`}
`}
`;

const StyledSubmenu = styled.ul<StyledSubmenuProps>`
Expand All @@ -73,6 +79,7 @@ const StyledSubmenu = styled.ul<StyledSubmenuProps>`
css`
box-shadow: var(--boxShadow100);
position: absolute;
top: 100%;
background-color: ${variant === "default"
? menuConfigVariants[menuType].submenuItemBackground
: menuConfigVariants[menuType].background};
Expand Down Expand Up @@ -212,10 +219,6 @@ const StyledSubmenu = styled.ul<StyledSubmenuProps>`
> a,
> button {
padding: 11px 16px 12px;
:has([data-component="icon"]) {
padding: 9px 16px 7px;
}
}
`}
Expand Down
10 changes: 5 additions & 5 deletions src/components/menu/component.test-pw.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export const MenuComponent = (props: Partial<MenuProps> & MenuDividerProps) => {
<Typography variant="h4" textTransform="capitalize" my={2}>
{menuType}
</Typography>
<Menu menuType={menuType} display="flex" {...props}>
<Menu menuType={menuType} {...props}>
<MenuItem href="#">Menu Item One</MenuItem>
<MenuItem href="#">Menu Item Two</MenuItem>
<MenuItem submenu="Menu Item Three">
Expand Down Expand Up @@ -422,7 +422,7 @@ export const MenuComponentItems = (
return (
<Box mb={150}>
<Typography textTransform="capitalize" my={2} />
<Menu menuType="white" display="flex">
<Menu menuType="white">
<MenuItem submenu="Menu Item One" submenuDirection="right" {...props}>
<MenuItem href="#">Item Submenu One</MenuItem>
<MenuItem href="#">Item Submenu Two</MenuItem>
Expand Down Expand Up @@ -622,7 +622,7 @@ export const MenuSegmentTitleComponent = (props: Partial<MenuTitleProps>) => {
<Typography variant="h4" textTransform="capitalize" my={2}>
{menuType}
</Typography>
<Menu menuType={menuType} display="flex">
<Menu menuType={menuType}>
<MenuItem href="#">Menu Item One</MenuItem>
<MenuItem href="#">Menu Item Two</MenuItem>
<MenuItem submenu="Menu Item Three">
Expand Down Expand Up @@ -655,7 +655,7 @@ export const MenuSegmentTitleComponentWithAdditionalMenuItem = (
<Typography variant="h4" textTransform="capitalize" my={2}>
{menuType}
</Typography>
<Menu menuType={menuType} display="flex">
<Menu menuType={menuType}>
<MenuItem href="#">Menu Item One</MenuItem>
<MenuItem href="#">Menu Item Two</MenuItem>
<MenuItem submenu="Menu Item Three">
Expand Down Expand Up @@ -705,7 +705,7 @@ export const MenuDividerComponent = (props: MenuDividerProps) => {
<Typography variant="h4" textTransform="capitalize" my={2}>
{menuType}
</Typography>
<Menu menuType={menuType} display="flex">
<Menu menuType={menuType}>
<MenuItem href="#">Menu Item One</MenuItem>
<MenuItem href="#">Menu Item Two</MenuItem>
<MenuItem submenu="Menu Item Three">
Expand Down
10 changes: 9 additions & 1 deletion src/components/menu/menu-item/menu-item.component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export type VariantType = "default" | "alternate";

interface MenuItemBaseProps
extends Omit<TagProps, "data-component">,
LayoutProps,
Pick<LayoutProps, "width" | "maxWidth" | "minWidth">,
FlexboxProps,
PaddingProps {
/** Custom className */
Expand Down Expand Up @@ -131,6 +131,13 @@ export const MenuItem = ({
"If no text is provided an `ariaLabel` should be given to facilitate accessibility."
);

invariant(
typeof submenu === "boolean" ||
submenu === undefined ||
(children && typeof submenu === "string" && submenu.length),
"You should not pass `children` when `submenu` is an empty string"
);

const {
inFullscreenView,
registerItem,
Expand Down Expand Up @@ -330,6 +337,7 @@ export const MenuItem = ({
{...paddingProps}
asDiv={hasInput || as === "div"}
hasInput={hasInput}
inSubmenu={!!handleSubmenuKeyDown}
>
{children}
</StyledMenuItemWrapper>
Expand Down
83 changes: 61 additions & 22 deletions src/components/menu/menu-item/menu-item.style.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ interface StyledMenuItemWrapperProps
asDiv?: boolean;
hasInput?: boolean;
menuItemVariant?: Pick<MenuWithChildren, "variant">["variant"];
inSubmenu?: boolean;
}

const BASE_SPACING = 16;
Expand Down Expand Up @@ -98,6 +99,7 @@ const StyledMenuItemWrapper = styled.a.attrs({
asPassiveItem,
asDiv,
hasInput,
inSubmenu,
}) => css`
display: flex;
align-items: center;
Expand All @@ -110,6 +112,9 @@ const StyledMenuItemWrapper = styled.a.attrs({
a,
button {
cursor: pointer;
min-height: 40px;
height: 100%;
box-sizing: border-box;
}
a:focus,
Expand All @@ -130,7 +135,7 @@ const StyledMenuItemWrapper = styled.a.attrs({
button:has([data-component="icon"]):not(:has(button))
${StyledContent} {
position: relative;
top: -2px;
top: -1px;
}
`}
Expand All @@ -155,11 +160,16 @@ const StyledMenuItemWrapper = styled.a.attrs({
${!inFullscreenView &&
css`
max-width: inherit;
width: inherit;
height: inherit;
> a,
> button {
display: flex;
align-items: center;
${!inSubmenu ? "justify-content: center;" : ""}
width: inherit;
max-width: inherit;
}
&& {
Expand Down Expand Up @@ -211,15 +221,29 @@ const StyledMenuItemWrapper = styled.a.attrs({
${
!inFullscreenView &&
`
> a:not(:has(button)) {
padding: 11px 16px 12px;
}
> a:not(:has(button)) {
padding: 11px 16px;
}
> a ${StyledButton}:not(.search-button) {
min-height: 17px;
padding: 9px 0px 11px;
}
`
> a:has(${StyledButton}:not(.search-button)) {
height: 100%;
${StyledContent} {
height: inherit;
div {
height: inherit;
}
}
${StyledButton} {
min-height: 40px;
padding: 10px 0px;
box-sizing: border-box;
height: 100%;
}
}
`
}
${StyledIconButton} {
Expand All @@ -237,29 +261,42 @@ const StyledMenuItemWrapper = styled.a.attrs({
}
`
: `
a,
${StyledLink} a,
button,
${StyledLink} button {
padding: ${inFullscreenView ? "0px 16px" : "11px 16px 12px"};
:has([data-component="icon"]) {
padding: 9px 16px 7px;
}
${
hasSubmenu || maxWidth
? `
a,
${StyledLink} a,
button,
${StyledLink} button {
padding: 11px 16px ${hasSubmenu && maxWidth ? "12px" : "10px"};
}
`
: `
a,
${StyledLink} a,
button,
${StyledLink} button {
padding: ${!inFullscreenView ? "11px" : "0px"} 16px;
}
`
}
`}
button,
${StyledLink} button {
height: 40px;
${StyledLink} button,
a,
${StyledLink} a {
margin: 0px;
text-align: left;
${inFullscreenView &&
css`
height: auto;
white-space: normal;
${StyledIcon} {
top: -2px;
}
`}
}
Expand All @@ -279,6 +316,8 @@ const StyledMenuItemWrapper = styled.a.attrs({
css`
a > ${StyledIcon}, button > ${StyledIcon} {
display: inline-block;
height: 18px;
top: -2px;
}
`}
}
Expand Down Expand Up @@ -396,7 +435,7 @@ const StyledMenuItemWrapper = styled.a.attrs({
a::before,
button::before {
display: block;
margin-top: -2px;
margin-top: -1px;
pointer-events: none;
position: absolute;
right: ${(props) => parsePadding(padding(props)).iconSpacing};
Expand Down
Loading
Loading