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

[Dialog] Fix animated dialog warning #786

Merged
merged 2 commits into from
Apr 21, 2021
Merged

[Dialog] Fix animated dialog warning #786

merged 2 commits into from
Apr 21, 2021

Conversation

mbellagamba
Copy link
Contributor

Thank you for contributing to Reach UI! Please fill in this template before submitting your PR to help us process your request more quickly.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code (Compile and run).
  • Add or edit tests to reflect the change (Run with yarn test).
  • Add or edit Storybook examples to reflect the change (Run with yarn start).
  • Ensure formatting is consistent with the project's Prettier configuration.
  • Add documentation to support any new features.

This pull request:

  • Creates a new package
  • Fixes a bug in an existing package
  • Adds additional features/functionality to an existing package
  • Updates documentation or example code
  • Other

If creating a new package:

  • Make sure the new package directory contains each of the following, and that their structure/formatting mirrors other related examples in the project:
    • examples directory
    • src directory with an index.tsx entry file
    • At least one example file per feature introduced by the new package
    • Base styles in a style.css file (if needed by the new package)

Fix the issue #669.

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a1104a8:

Sandbox Source
reach-ui-template Configuration

@mbellagamba mbellagamba changed the title [Dialog] Fix animated dialog warning (#669) [Dialog] Fix animated dialog warning Apr 16, 2021
@chaance
Copy link
Member

chaance commented Apr 21, 2021

Can you clarify what warning you're fixing here? I don't actually see an issue with the current example in the docs.

@chaance chaance added Status: Awaiting Response Requested and awaiting a response from the issue creator Type: Documentation Changes to the docs labels Apr 21, 2021
@mbellagamba
Copy link
Contributor Author

It fixes memory leak warning described in the issue #669. The CodeSandbox provided with the issue shows two console errors clicking on the button:

  1. Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.
  2. Warning: Failed prop type: A must have either an aria-label or aria-labelledby prop.

Both errors are shown in the console even when clicking the button that shows the animated dialog on the documentation website.
Moving animated components definition outside the Dialog component fixes the memory leak warning. Then I added an heading to fix the missing dialog label warning. You can see a working example in this CodeSandbox. They are not critical warning but I believe it's better to fix them. Anyway I'm open to further clarification if needed.

@chaance chaance self-requested a review April 21, 2021 21:21
@chaance chaance merged commit 54764b1 into reach:develop Apr 21, 2021
@mbellagamba mbellagamba deleted the fix-animated-dialog branch April 27, 2021 05:50
chaance pushed a commit that referenced this pull request Jul 10, 2021
)

* dialog: fix warning in animated example docs
* dialog: add animated example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Awaiting Response Requested and awaiting a response from the issue creator Type: Documentation Changes to the docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants