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

Show upload error in modal, not in toast #1126

Merged
merged 1 commit into from
Feb 25, 2025

Conversation

edlerd
Copy link
Collaborator

@edlerd edlerd commented Feb 25, 2025

Done

  • Show upload error in modal, not in toast, as that will not be visible with an open modal

QA

  1. Run the LXD-UI:
  2. Perform the following QA steps:
    • upload a custom iso
    • upload the same file again, the error should displayed inside the modal, not as toast that is behind the modal.

@webteam-app
Copy link

mas-who
mas-who previously approved these changes Feb 25, 2025
Copy link
Contributor

@mas-who mas-who left a comment

Choose a reason for hiding this comment

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

Small QA nit, happy to merge as is:

I think it's a little weird to have the error notification showing inside the modal as well as the custom isos list page. The upload error seems to be scoped to the modal.
Screenshot from 2025-02-25 12-31-35

Maybe we can check in CustomIsoList if the upload modal is open and conditionally render NotificationRow?

…e visible with an open modal

Signed-off-by: David Edler <[email protected]>
@edlerd
Copy link
Collaborator Author

edlerd commented Feb 25, 2025

I think it's a little weird to have the error notification showing inside the modal as well as the custom isos list page. The upload error seems to be scoped to the modal.

Right, seems better to have a custom error inside the modal, than to rely on the notify hook for that reason. Change the PR slightly to adjust to it.

@edlerd edlerd requested a review from mas-who February 25, 2025 11:33
Copy link
Contributor

@mas-who mas-who left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the adjustment

@edlerd edlerd merged commit 5d981ff into canonical:main Feb 25, 2025
11 checks passed
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.

3 participants