Skip to content

[Dialog] Fix animated dialog warning#786

Merged
chaance merged 2 commits intoreach:developfrom
mbellagamba:fix-animated-dialog
Apr 21, 2021
Merged

[Dialog] Fix animated dialog warning#786
chaance merged 2 commits intoreach:developfrom
mbellagamba:fix-animated-dialog

Conversation

@mbellagamba
Copy link
Copy Markdown
Contributor

@mbellagamba mbellagamba commented Apr 16, 2021

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
Copy Markdown

codesandbox-ci Bot commented Apr 16, 2021

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

mbellagamba commented Apr 21, 2021

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
@chaance chaance mentioned this pull request Apr 21, 2021
Closed
@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
@mbellagamba @chaance
[Dialog] Fix animated dialog example to remove memory leak warning (#786
7056def 
)

* 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

Reviewers

@chaance chaance chaance approved these changes

Assignees

No one assigned

Labels

Status: Awaiting Response Requested and awaiting a response from the issue creator Type: Documentation Changes to the docs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mbellagamba @chaance