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

[chore] update readme and docs demos #1012

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

neilhsmith
Copy link

Closes #955.

Changes proposed:

  • Update the readme and doc's demos. Updating to a modern version of react-modal fixes the accessibility issues noted in the issue automatically.
  • Remove the setAppElement example. A warning is now logged when the app element is not set so removed this one and instead setAppElement in all other demos.
  • Add a new styled-components example.

The demos themselves have remained largely unchanged apart from converting to functional components. They are still styled simply and do not set any extra props. All demos are stored in this CodePen collection.

Upgrade Path (for changed or removed APIs):

Acceptance Checklist:

  • The commit message follows the guidelines in CONTRIBUTING.md.
  • Documentation (README.md) and examples have been updated as needed.
  • If this is a code change, a spec testing the functionality has been added.
  • If the commit message has [changed] or [removed], there is an upgrade path above.

@@ -1,7 +0,0 @@
# Using setAppElement
Copy link
Collaborator

@diasbruno diasbruno Apr 8, 2023

Choose a reason for hiding this comment

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

Why was this removed?

Copy link
Author

Choose a reason for hiding this comment

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

Maybe I'm missing historical context but I removed it because it feels more of a requirement than a suggestion since a large warning is logged to the console when it's omitted and since every other example already included it. But if it's needed, then sure, I'll add it back in just a few.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe the explaination is bad. setAppElement is a feature that is misused all the time.
Problem is that users always place the setAppElement together with the modal utilization,
which causes problems when testing and others (i'll try to reference them).
Suggestions are welcome.

Copy link
Author

Choose a reason for hiding this comment

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

OK, added back in. No strong suggestions, I just assumed setAppElement wasn't as strongly recommended in the past or something and might not be needed today.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually setAppElement must be used. Historically the default element that react-modal uses to add the aria-hidden attribute is wrong. By default it will use the document.body which will turn the whole page inaccessible.

@diasbruno
Copy link
Collaborator

Remove the setAppElement example. A warning is now logged when the app element is not set so removed this one and instead setAppElement in all other demos.

This needs to come back. setAppElement has been a hard feature to explain to users.

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.

Accessibility issues with the codepen demos
2 participants