Skip to content

feat(Modal): Added optional Id for close button #6732

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

Merged
jpuzz0 merged 5 commits into from
Jan 20, 2022
Merged

feat(Modal): Added optional Id for close button #6732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9e3fbc8
feat(Modal): Added optional Id for close button
Jan 4, 2022
a898b97
Updated to use a more flexible prop
Jan 5, 2022
19f81f3
add ouid to button
Jan 18, 2022
46a6dd0
only add ouiaId when not undefined
Jan 18, 2022
3e14050
update snapshot test
Jan 18, 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
13 changes: 11 additions & 2 deletions packages/react-core/src/components/Modal/ModalBoxCloseButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import * as React from 'react';
import { Button } from '../Button';
import TimesIcon from '@patternfly/react-icons/dist/esm/icons/times-icon';
import { OUIAProps } from '../../helpers';

export interface ModalBoxCloseButtonProps {
export interface ModalBoxCloseButtonProps extends OUIAProps {
/** Additional classes added to the close button */
className?: string;
/** A callback for when the close button is clicked */
Expand All @@ -12,9 +13,17 @@ export interface ModalBoxCloseButtonProps {
export const ModalBoxCloseButton: React.FunctionComponent<ModalBoxCloseButtonProps> = ({
className = '',
onClose = () => undefined as any,
ouiaId,
...props
}: ModalBoxCloseButtonProps) => (
<Button className={className} variant="plain" onClick={onClose} aria-label="Close" {...props}>
<Button
className={className}
variant="plain"
onClick={onClose}
aria-label="Close"
{...(ouiaId && { ouiaId: `${ouiaId}-${ModalBoxCloseButton.displayName}` })}
Copy link
Copy Markdown
Contributor

@jpuzz0 jpuzz0 Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not familiar with OUIAProps yet, but does this usage of ouiaId meet the user requirements to be able to specify a custom data-ouia-component-id? Maybe they're synonymous?

Copy link
Copy Markdown
Contributor Author

@tlabaj tlabaj Jan 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@jeffpuzzo Yes. I explain in the description for this PR how you can see this demonstrated.
I talked to @jgiardino and Tiffany offline and what they were actually looking for is to utilize the ouiaId for identification purposes. The modal is already enabled to use ouiaId. Since the Modal is not compassable, the consumer can not add ouiaId to the close button. I pass the model's ouiaId to the close button and append the close button's display name to it so it is unique then I add the ouiaId to the button.
You can read more about OUIA here

{...props}
>
<TimesIcon />
</Button>
);
Expand Down
2 changes: 1 addition & 1 deletion packages/react-core/src/components/Modal/ModalContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ export const ModalContent: React.FunctionComponent<ModalContentProps> = ({
aria-describedby={ariaDescribedby || (hasNoBodyWrapper ? null : descriptorId)}
{...getOUIAProps(ModalContent.displayName, ouiaId, ouiaSafe)}
>
{showClose && <ModalBoxCloseButton onClose={onClose} />}
{showClose && <ModalBoxCloseButton onClose={onClose} ouiaId={ouiaId} />}
{modalBoxHeader}
{modalBody}
{modalBoxFooter}
Expand Down