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

feat(PPDSC-2403): Extend role and aria-haspopup on Popover #381

Merged
merged 22 commits into from
Sep 28, 2022
Merged

feat(PPDSC-2403): Extend role and aria-haspopup on Popover #381

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
17c9627
feat(PPDSC-2403): added role in props
Sep 20, 2022
ff0a16f
feat(PPDSC-2403): removed unused code
Sep 20, 2022
1003e87
feat(PPDSC-2403): disable the default aria-haspopup
Sep 20, 2022
59149ae
feat(PPDSC-2403): code cleanup
Sep 20, 2022
bb18f1f
feat(PPDSC-2403): code clean up
Sep 21, 2022
efe1e52
feat(PPDSC-2403): code cleanup
Sep 21, 2022
88b1de2
feat(PPDSC-2403): code cleanup
Sep 21, 2022
74597b5
feat(PPDSC-2403): renamed the props
Sep 21, 2022
cff1a1c
feat(PPDSC-2403): type refinement
Sep 21, 2022
1e513f9
feat(PPDSC-2403): removed duplicating the role
Sep 21, 2022
f95170f
feat(PPDSC-2403): code cleanup
Sep 21, 2022
518d767
feat(PPDSC-2403): linting fix
Sep 21, 2022
9a958cd
feat(PPDSC-2403): linting fix
Sep 21, 2022
1e8cff0
feat(PPDSC-2403): unit test case error fix
Sep 21, 2022
9627ac9
feat(PPDSC-2403): code patch fix
Sep 21, 2022
f7e473d
feat(PPDSC-2403): test case
Sep 23, 2022
779f761
feat(PPDSC-2403): simplified the condition
Sep 26, 2022
da16681
feat(PPDSC-2403): simplified the condition
Sep 26, 2022
061cc94
feat(PPDSC-2403): removed the commented codes
Sep 27, 2022
9e77e25
feat(PPDSC-2403): removed the commented codes
Sep 27, 2022
e179bf7
feat(PPDSC-2403): removed the commented codes
Sep 27, 2022
41877f2
feat(PPDSC-2403): removed the commented codes
Sep 28, 2022
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
1 change: 1 addition & 0 deletions src/base-floating-element/base-floating-element.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ export const BaseFloatingElement = React.forwardRef<
id={`${floatingId}-pointer`}
ref={pointerRef}
placement={statefulPlacement}
role={role}
$x={pointerX}
$y={pointerY}
overrides={overrides}
Expand Down
1 change: 1 addition & 0 deletions src/base-floating-element/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export interface FloatingElementProps
| ((referenceProps: ReferenceProps) => React.ReactNode);
open?: boolean;
placement?: Placement;
role?: AriaRole;
overrides?: {
zIndex?: number;
maxWidth?: MQ<string>;
Expand Down
1 change: 1 addition & 0 deletions src/popover/__tests__/popover.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ const PopoverWithBtn = (
aria-label="info-icon"
size="small"
overrides={{stylePreset: 'iconButtonOutlinedPrimary'}}
aria-haspopup="true"
>
<IconFilledInfo />
</IconButton>
Expand Down
5 changes: 3 additions & 2 deletions src/popover/popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {filterOutFalsyProperties} from '../utils/filter-object';
const buildContextAriaAttributes: BuildAriaAttributesFn = ({
floating: {id, open},
}) => ({
'aria-haspopup': 'dialog',
// 'aria-haspopup': 'dialog',
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to remove that, this should stay as the default behaviour.

but we need to check if people provide they aria-haspopup and if so we don't apply it automatically.

For example:

<Popover content="..." role="menu">
  <IconButton aria-haspopup="true" />
</Popover>

That logic should happen in base-floating-element.tsx file, at the point you are cloning the children you can check if the children have already aria-popup

'aria-controls': open ? id : undefined,
});

Expand All @@ -40,6 +40,7 @@ const ThemelessPopover = React.forwardRef<HTMLDivElement, PopoverProps>(
header,
closePosition = 'right',
overrides = {},
role,
handleCloseButtonClick,
enableDismiss = false,
disableFocusManagement = false,
Expand Down Expand Up @@ -122,7 +123,7 @@ const ThemelessPopover = React.forwardRef<HTMLDivElement, PopoverProps>(
buildRefElAriaAttributes={buildContextAriaAttributes}
buildFloatingElAriaAttributes={buildFloatingElementAriaAttributes}
useInteractions={useInteractions}
role="dialog"
role={role}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the description of the ticket might be misleading.
you don't need to change this, people can pass a role on the popover and this will be overridden.

For example, I just did that with a passing banner:
Screenshot 2022-09-21 at 9 12 41

overrides={overrides}
disableFocusManagement={disableFocusManagement}
{...props}
Expand Down
1 change: 1 addition & 0 deletions src/popover/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export type PopoverProps = Omit<
children: React.ReactNode;
content: React.ReactNode;
closePosition?: 'left' | 'right' | 'none';
// popoverrole?: AriaRole;
header?: React.ReactNode;
handleCloseButtonClick?: () => void;
enableDismiss?: boolean;
Expand Down