Skip to content

Commit

Permalink
fix: address PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
booc0mtaco committed Nov 22, 2022
1 parent b629157 commit f47f4c8
Show file tree
Hide file tree
Showing 10 changed files with 30 additions and 15 deletions.
5 changes: 4 additions & 1 deletion src/components/ButtonDropdown/ButtonDropdown.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import styles from './ButtonDropdown.module.css';
import { DropdownMenu } from '../..';
import type { ClickableStyleProps } from '../ClickableStyle';

// TODO: This component is deprecated and will be replaced by the Menu component
export interface Props {
/**
* Aria label to be attacehd to the dropdown trigger button.
Expand Down Expand Up @@ -63,9 +62,13 @@ export interface Props {
}

/**
* The ButtonDropdown component is deprecated and will be removed in a future release.
*
* `import {ButtonDropdown} from "@chanzuckerberg/eds";`
*
* Contains the button and the dropdown.
*
* @deprecated
*/
export const ButtonDropdown = ({
fullWidth,
Expand Down
8 changes: 4 additions & 4 deletions src/components/Menu/Menu.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@
}

.menu__button {
background-color: var(--light-color-button-background);
background-color: var(--eds-theme-color-form-background);
}

/* // is getting overridden by a style in clickablestyle */
.menu__button--icon {
color: var(--eds-theme-color-button-secondary-neutral-text);
/* Needed to create higher specificity than ClickableStyle. TODO: improve */
.menu__button--icon.menu__button--icon.menu__button--icon {
color: var(--eds-theme-color-icon-neutral-default);
}

.menu__item {
Expand Down
2 changes: 1 addition & 1 deletion src/components/Menu/Menu.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export const Default: StoryObj<Args> = {
render: (args) => (
<Menu {...args}>
<Menu.Button>Documentation Links</Menu.Button>
<Menu.Items>
<Menu.Items data-testid="menu-content">
<Menu.Item
href="https://headlessui.com/react/menu#menu-button"
icon="link"
Expand Down
4 changes: 2 additions & 2 deletions src/components/Menu/Menu.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Menu as HeadlessMenu } from '@headlessui/react';
import clsx from 'clsx';
import React from 'react';
import type { MouseEventHandler } from 'react';

import styles from './Menu.module.css';
import type { ExtractProps } from '../../util/utility-types';
Expand All @@ -22,7 +23,7 @@ export type MenuProps = ExtractProps<typeof HeadlessMenu> & {
export type MenuItemProps = ExtractProps<typeof HeadlessMenu.Item> & {
href?: string;
icon?: IconName;
onClick?: () => any;
onClick?: MouseEventHandler<HTMLAnchorElement>;
};

export type MenuButtonProps = ExtractProps<typeof HeadlessMenu.Button>;
Expand Down Expand Up @@ -74,7 +75,6 @@ const MenuItems = (props: MenuItemsProps) => (
<HeadlessMenu.Items
as={PopoverContainer}
className={clsx(styles['menu__popover'])}
data-testid="menu-content"
{...props}
/>
);
Expand Down
1 change: 0 additions & 1 deletion src/components/Popover/Popover.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ export const Default: StoryObj<Args> = {

export const Arrow: StoryObj<Args> = {
args: {
// maybe gets shallow merged with the default args (?)
children: (
<>
<Popover.Button as={Button}>Open Popover</Popover.Button>
Expand Down
1 change: 0 additions & 1 deletion src/components/Popover/Popover.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ describe('<Popover />', () => {
});

it('should open Popover with trigger button', () => {
// this test is redundant b/c of the snapshot above
render(<Default />);
expect(screen.queryByTestId('popover-content')).not.toBeInTheDocument();
const triggerButton = screen.getByTestId('popover-trigger-button');
Expand Down
2 changes: 1 addition & 1 deletion src/components/PopoverListItem/PopoverListItem.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

.popover-list-item--disabled {
color: var(--eds-theme-color-text-disabled);
pointer-events: none;
cursor: not-allowed;
}

.popover-list-item__icon {
Expand Down
2 changes: 1 addition & 1 deletion src/components/PopoverListItem/PopoverListItem.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ type Args = React.ComponentProps<typeof PopoverListItem>;

export const Default: StoryObj<Args> = {
args: {
children: <>Default list item</>,
children: 'Default list item',
},
};

Expand Down
19 changes: 16 additions & 3 deletions src/components/PopoverListItem/PopoverListItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ export interface Props {
* Whether the list item is treated as highlighted in its container
*/
active?: boolean;
/**
* Child node(s) that can be nested inside component
*/
children: ReactNode;
/**
* Whether the list item is treated as disabled
Expand All @@ -31,7 +34,7 @@ export interface Props {
*
* `import {PopoverListItem} from "@chanzuckerberg/eds";`
*
* This abstracts the structure of a item in a popover, when the popover contains a
* This abstracts the structure of an item in a popover, when the popover contains a
* list of items (e.g., Menus and Selects)
* - Contains styles for when active/disabled or not
* - contains styles for when there is an icon on the left
Expand All @@ -48,9 +51,19 @@ export const PopoverListItem = React.forwardRef<HTMLDivElement, Props>(
className,
);

// use `as` to use a/button when appropriate
const ariaIsDisabled = disabled
? {
'aria-disabled': true,
}
: {};

return (
<div className={componentClassName} {...other} ref={ref}>
<div
className={componentClassName}
{...other}
{...ariaIsDisabled}
ref={ref}
>
{icon ? (
<div className={styles['popover-list-item__icon']}>
<Icon name={icon} purpose="decorative" size="1.5rem" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ exports[`<PopoverListItem /> Default story renders snapshot 1`] = `

exports[`<PopoverListItem /> Disabled story renders snapshot 1`] = `
<div
aria-disabled="true"
class="popover-list-item popover-list-item--disabled"
>
<div
Expand Down

0 comments on commit f47f4c8

Please sign in to comment.