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(uploads): fix archive upload modal invalid labels popover #998

Merged

Conversation

maxcao13
Copy link
Member

@maxcao13 maxcao13 commented Apr 26, 2023

Welcome to Cryostat! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed the last commit: git commit --amend --signoff

Fixes: #997

Description of the change:

The change reverts ArchiveUploadModal to append to document body for the popover to show again. I cannot figure out quite the reason why appending the modal to portalRoot makes popperjs think that the popper is being cutoff in some way.

According to an article I read:

Popper attaches attributes in the following cases:

    data-popper-escaped: When the popper escapes the reference element's clipping container (it appears detached)
    data-popper-reference-hidden: When the reference element is hidden from view (it appears attached to nothing)

But it doesn't seem the reference is ever hidden from view, even when the expandedContent is expanded, and it doesn't make sense why having the modal append to portalRoot vs root/body would add these attributes.

For now, appending the modal to document body is completely ok.

The change also shows the popover if invalid jsons that dont have the label field are uploaded.

How to manually test:

  1. go to Archives -> Upload -> Test out bad metadata json files e.g. {ASDfsadf} and {"labels":{"asdf"""":"asdf"}}

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-998-ce2be7cb29ab331f37cf9e07f1808e41024bc9ef sh smoketest.sh

Copy link
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

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

Looks good! Very weird with Popper.js indeed.

@andrewazores andrewazores merged commit ef3cb3f into cryostatio:main Apr 26, 2023
mergify bot pushed a commit that referenced this pull request Apr 26, 2023
andrewazores pushed a commit that referenced this pull request Apr 26, 2023
…999)

(cherry picked from commit ef3cb3f)

Co-authored-by: Max Cao <macao@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Bug] Error popover is not showing when uploading labels
3 participants