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

build(deps): revert @patternfly/react-table from 4.112.39 to 4.112.6 #864

Closed

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Feb 7, 2023

This reverts commit 943460c.

Signed-off-by: Andrew Azores aazores@redhat.com

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

Related to #863

Description of the change:

This reverts #858 , which introduced some styling change that causes views containing tables to render strangely on 1920x1200 displays (or anything not 16:9 perhaps).

Motivation for the change:

This is a short-term fix for the bug referenced. Ideally we can upgrade to that @patternfly/react-table version or a later one. There may be some css fix or similar that we need to apply locally on top of the version upgrade. This may just be a Patternfly bug in the end which we should report.

How to manually test:

Simply resizing the viewport doesn't seem to trigger the bug, so I'm not sure how to test this without having access to a non-16:9 display. With such a display, just open the web-client and go to ex. Recordings and check if the negative space at the bottom of the page is present.

@andrewazores andrewazores requested a review from ebaron as a code owner February 7, 2023 14:02
@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Feb 7, 2023
@andrewazores andrewazores requested a review from tthvo February 7, 2023 14:02
@mergify mergify bot added the safe-to-test label Feb 7, 2023
@andrewazores andrewazores removed the needs-triage Needs thorough attention from code reviewers label Feb 7, 2023
This reverts commit 943460c.

Signed-off-by: Andrew Azores <aazores@redhat.com>
@github-actions
Copy link

github-actions bot commented Feb 7, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-864-dda719fcd5d2cab529a4fb3d5b8e751d63efbb49 sh smoketest.sh

@andrewazores andrewazores requested a review from maxcao13 February 7, 2023 14:12
Copy link
Member

@maxcao13 maxcao13 left a comment

Choose a reason for hiding this comment

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

Something weird going on. I have a problem where the yarn tests fail because my PR revert the snapshot change to "OUIA-Generated-Button-plain-5", just like this revert does. I will approve, it may solve my issue as well.

@tthvo
Copy link
Member

tthvo commented Feb 8, 2023

maybe we should hold off to see if we can resolve this #863 (comment). The Rule table needs this version in order for the ActionColumns to have menuAppendTo added...

@tthvo
Copy link
Member

tthvo commented Feb 21, 2023

@andrewazores Hey, looks like this react-table 4.112.39 is requiring core ^4.276.6.

While quickstarts only uses ^4.267.6. This causes #881 (review) to fail.

Not sure how long it takes for quickstarts to bump core so we really need to revert this? Tho, we will need a custom ActionColumn as lower versions of react-table does not export menuAppendTo prop.

@tthvo
Copy link
Member

tthvo commented Feb 21, 2023

This causes #881 (review) to fail.

On some closer look, I am not even sure why the core is automatically pumped to 2.276.6 (checked with yarn info). I just suspect this react-table cuz it seems to be the only place that requires this core version along with catalog-extension (deps from quickstarts).

@patternfly/react-core@npm:^4.267.6 → npm:4.276.6 [af275]

@andrewazores
Copy link
Member Author

We still have some time to make the call of whether to revert this or not, so let's wait a while longer and see what happens with those different Patternfly packages and see if their interdependencies sort themselves out. Or we can just ask the Patternfly team what's going on and what the release cadence is for things.

@tthvo
Copy link
Member

tthvo commented Feb 21, 2023

Ahh I am seeing some peer deps are being pumped to later versions what requires 4.276.6. On a second thought, we might need to bump our PF core (#851) and keep close this PR.

The only problem is that quickstart package is not yet bumping their core. I will open an issue on their repo to see.

Open here: patternfly/patternfly-quickstarts#227. Best case we can bump to 4.276.6 and keep close this PR.

@andrewazores
Copy link
Member Author

Awesome, thanks for following up on that.

@tthvo
Copy link
Member

tthvo commented Apr 6, 2023

Maybe we can close this since the css order is now fixed?

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.

3 participants