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

fix(Modal): Various bug fixes #11168

Open
wants to merge 3 commits into
base: v5
Choose a base branch
from
Open

Conversation

tlabaj
Copy link
Contributor

@tlabaj tlabaj commented Nov 7, 2024

What: Closes #11041

This fixes:

  • Modal container getting aria-hidden = true when in StrictMode
  • Modal adjacent siblings getting aria-hidden = true incorrectly removed
  • Modal div getting created and not removed until refresh
  • This also removes the wrapping container div that was use to append the modal to when creating a portal. Instead, we append directly to the documentation body by default (Newer react documentation shows this as an example). The consumer can still pass an element to append to the via the appendTo prop.

I also added .history to the eslint ignore.

The test example can be used to test out the Modal changes here and the Modal Next changes here. You can compare the examples with the original issue in the code sandbox that was provided by the reporter https://codesandbox.io/s/withered-frog-qc67dn?file=/index.js
cc @christianvogt

TODO:

  • Remove the test examples
    - Make the same changes to the Modal next
  • In the main branch make changes to the Modal and deprecated Modal.

@patternfly-build
Copy link
Contributor

patternfly-build commented Nov 7, 2024

@kmcfaul
Copy link
Contributor

kmcfaul commented Nov 8, 2024

I'm a little confused by the test examples' text and want to confirm that we should not be seeing aria-hidden={false} on non-hidden modals. The text indicates to look for the attribute but it isn't present. And for the adjacent modal example, the text says to look for closed modals' attributes but the closed modals aren't in the DOM - which the PR indicates should be the case.

I think both behaviors make sense but want to make sure I'm not misunderstanding. If they are correct, then this PR LGTM.

@tlabaj
Copy link
Contributor Author

tlabaj commented Nov 8, 2024

@kmcfaul the example are directly copied from the code sandbox in the issue. SO the text in the example talks about the bug that was fixed. I agree that is confusing. I can update it.

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.

3 participants