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: resolve notification provider issue with react-router-dom upgrade for good #614

Closed
wants to merge 1 commit into from

Conversation

mas-who
Copy link
Contributor

@mas-who mas-who commented Jan 19, 2024

Done

  • This is a sister PR for the react-components fix in NotificationProvider. Instead of calling useLocation in NotificationProvider, we will now pass the returned state and pathname as props to the provider component.
  • Upgraded react-router-dom to 6.21.3

QA

  1. Run the LXD-UI:
    • On the demo server via the link posted by @webteam-app below. This is only available for PRs created by collaborators of the repo. Ask @lorumic or @edlerd for access.
    • With a local copy of this branch, run as described here.
  2. Perform the following QA steps:
    • Wait until the react component PR gets merged, then check that queued notifications still work.

@webteam-app
Copy link

Demo starting at https://lxd-ui-614.demos.haus

…e for good

Signed-off-by: Mason Hu <mason.hu@canonical.com>
Copy link
Collaborator

@edlerd edlerd Jan 20, 2024

Choose a reason for hiding this comment

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

There's only a single use case of this and we don't expect more, so why not include the changes directly in the Root component?
Similar solution in #616

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's only a single use case of this and we don't expect more, so why not include the changes directly in the Root component? Similar solution in #616

It was just a personal preference, since only notification provider needed state and path from useLocation I decided to keep it close to notification provider instead of Root. But your approach makes more sense 👍

@mas-who
Copy link
Contributor Author

mas-who commented Jan 22, 2024

Closing this PR as the same issue was addressed by PR #616

@mas-who mas-who closed this Jan 22, 2024
github-actions bot pushed a commit that referenced this pull request Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants