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.

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

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

Merged
merged 5 commits into from Jan 20, 2022

Conversation

tlabaj
Copy link
Contributor

@tlabaj tlabaj commented Jan 4, 2022

What: Closes #5856

Updated the ModalContent pass the ouiaId to the Modal close button. The Modal close button component appends the display name to the ouiaId and sets that as the button's ouiaId.

To test this add ouiaId="testId" to the in any example. This will add "testIdt-ModalBoxCloseButton"

@patternfly-build
Copy link
Contributor

patternfly-build commented Jan 4, 2022

@jpuzz0
Copy link
Contributor

jpuzz0 commented Jan 4, 2022

Looking at the initial issue, I see "We are currently using data- attributes to uniquely identify elements in the DOM.", where this is adding an id prop to the button, I'm wondering if this solves the problem for the user completely. It's certainly better than them using the className cited in the ticket though.

Should we confirm this meets the requirement?

@tlabaj
Copy link
Contributor Author

tlabaj commented Jan 4, 2022

Looking at the initial issue, I see "We are currently using data- attributes to uniquely identify elements in the DOM.", where this is adding an id prop to the button, I'm wondering if this solves the problem for the user completely. It's certainly better than them using the className cited in the ticket though.

Should we confirm this meets the requirement?

Yes. I will as Jenn to take a look. thanks!

@tlabaj tlabaj requested a review from jgiardino January 5, 2022 16:27
@jgiardino
Copy link
Collaborator

Thanks for picking up this issue!

@tiffanynolan has taken over the task of redefining how we tag UI elements so that we can uniquely identify them for the purpose of gathering user metrics, so I'd like her to weigh in on the solution provided here.

One question I have is whether it's possible to enable us to specify a custom data- html attribute instead of using the id attribute? TBH, I'm not sure how important it is that the html attributes we use for identifying UI elements are the same, but wanted to ask. If that's not an option, then the id should support our use case.

@tlabaj
Copy link
Contributor Author

tlabaj commented Jan 5, 2022

@jeffpuzzo @jgiardino @tiffanynolan. I updated this to add a prop closeButtonProps to modal. It is more flexible and you can add data-. attributes or an id.

Copy link
Collaborator

@jgiardino jgiardino left a comment

Choose a reason for hiding this comment

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

Thanks for updating this! This approach would definitely support our use case.

Copy link
Collaborator

@jgiardino jgiardino left a comment

Choose a reason for hiding this comment

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

Can I take back what I said 😬

Looking at what Tiffany proposed, she wants to use data-ouia-component-id as our attribute for tagging elements, which appears to already be used for this element. When I try to specify that value with this prop, it appears that I can't override the dynamic value that is generated by PF. Sorry for the back-track on this one.

@tlabaj tlabaj requested a review from jgiardino January 18, 2022 16:03
variant="plain"
onClick={onClose}
aria-label="Close"
{...(ouiaId && { ouiaId: `${ouiaId}-${ModalBoxCloseButton.displayName}` })}
Copy link
Contributor

@jpuzz0 jpuzz0 Jan 18, 2022

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
Contributor Author

@tlabaj tlabaj Jan 19, 2022

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

@jpuzz0 jpuzz0 self-requested a review January 19, 2022 16:40
@tiffanynolan
Copy link
Member

This LGTM! Much better and should work with our Pendo tracking.

Copy link
Collaborator

@jgiardino jgiardino left a comment

Choose a reason for hiding this comment

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

LGTM - thanks!

@jpuzz0 jpuzz0 merged commit 268db07 into patternfly:main Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to provide unique identifier for the Close button in a modal
8 participants