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.

Sign up for GitHub

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

Jump to bottom

Fix Tooltip Rendering Issue by Upgrading react-charts #1487

Closed
wants to merge 1 commit into from
Closed

Fix Tooltip Rendering Issue by Upgrading react-charts #1487

wants to merge 1 commit into from

Conversation

TimothyAsirJeyasing
Copy link
Contributor

This PR addresses the tooltip rendering issues
encountered in [email protected].
The issue has been resolved in [email protected].

https://bugzilla.redhat.com/show_bug.cgi?id=2281587

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jul 23, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: TimothyAsirJeyasing
Once this PR has been reviewed and has the lgtm label, please assign sanjalkatiyar for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@TimothyAsirJeyasing
Copy link
Contributor Author

TimothyAsirJeyasing commented Jul 23, 2024 •
edited
Loading

Screenshot from 2024-07-23 18-52-07
image

@GowthamShanmugam
Copy link
Contributor

/retest

@GowthamShanmugam
Copy link
Contributor

Please check the build failure

@TimothyAsirJeyasing
Copy link
Contributor Author

/rebuild

@SanjalKatiyar
Copy link
Collaborator

update shared dependencies as well...

@TimothyAsirJeyasing TimothyAsirJeyasing force-pushed the bz-2281587-tooltip-rendered-behind-other-components branch 2 times, most recently from 66714f1 to 8d1a050 Compare August 2, 2024 19:03
@SanjalKatiyar
Copy link
Collaborator

update shared dependencies as well...

this is still missing...

@SanjalKatiyar
Copy link
Collaborator

why do we need changes in analyse file ??

@TimothyAsirJeyasing TimothyAsirJeyasing force-pushed the bz-2281587-tooltip-rendered-behind-other-components branch from 8d1a050 to 5c24eea Compare August 7, 2024 10:57
@TimothyAsirJeyasing
Copy link
Contributor Author

/test odf-console-e2e-aws

@TimothyAsirJeyasing
Copy link
Contributor Author

why do we need changes in analyse file

We updated the analyzeTest.ts file to fix two key issues: First, after upgrading react-charts, the script needed adjustments to accurately analyze the new build artifacts and avoid misinterpretations during the build process. Second, it was encountering a memory limit error ("Cannot create a string longer than 0x1fffffe8 characters") when processing large files, which we've optimized.

@TimothyAsirJeyasing TimothyAsirJeyasing force-pushed the bz-2281587-tooltip-rendered-behind-other-components branch from 5c24eea to 088efff Compare August 9, 2024 13:14
@SanjalKatiyar
Copy link
Collaborator

We updated the analyzeTest.ts file to fix two key issues: First, after upgrading react-charts, the script needed adjustments to accurately analyze the new build artifacts and avoid misinterpretations during the build process. Second, it was encountering a memory limit error ("Cannot create a string longer than 0x1fffffe8 characters") when processing large files, which we've optimized.

You have not "optimized" anything, rather just increases the base size to make sure there are no errors while running the script. I don't think all the changes in the analyse file is necessary, plz check again.
#1520 should do the required optimisation.

@TimothyAsirJeyasing TimothyAsirJeyasing force-pushed the bz-2281587-tooltip-rendered-behind-other-components branch from 088efff to 3d0cfb1 Compare August 20, 2024 07:30
@TimothyAsirJeyasing
Copy link
Contributor Author

/retest

@SanjalKatiyar
Copy link
Collaborator

fix ur commits in this PR... also check why tests are failing...

@TimothyAsirJeyasing TimothyAsirJeyasing force-pushed the bz-2281587-tooltip-rendered-behind-other-components branch from 159e0cc to 95dd8eb Compare August 22, 2024 08:16
@TimothyAsirJeyasing
Copy link
Contributor Author

/test odf-console-e2e-aws

@TimothyAsirJeyasing TimothyAsirJeyasing force-pushed the bz-2281587-tooltip-rendered-behind-other-components branch from 95dd8eb to 5cd96ae Compare August 26, 2024 16:49
@GowthamShanmugam
Copy link
Contributor

/test odf-console-e2e-aws

@TimothyAsirJeyasing TimothyAsirJeyasing force-pushed the bz-2281587-tooltip-rendered-behind-other-components branch from 5cd96ae to e0e4caf Compare August 27, 2024 15:21
@TimothyAsirJeyasing
Copy link
Contributor Author

/retest

alfonsomthd
alfonsomthd reviewed Sep 2, 2024
View reviewed changes
package.json Outdated Show resolved Hide resolved
@TimothyAsirJeyasing
Fix Tooltip Rendering Issue by Upgrading react-charts
fdfd3fb 
This PR addresses the tooltip rendering issues
encountered in [email protected].
The issue has been resolved in [email protected].

https://bugzilla.redhat.com/show_bug.cgi?id=2281587
Signed-off-by: Timothy Asir Jeyasingh <[email protected]>
@TimothyAsirJeyasing TimothyAsirJeyasing force-pushed the bz-2281587-tooltip-rendered-behind-other-components branch from e0e4caf to fdfd3fb Compare September 5, 2024 12:07
@alfonsomthd
Copy link
Collaborator

/retest

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Sep 6, 2024

@TimothyAsirJeyasing: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/odf-console-e2e-aws fdfd3fb link true /test odf-console-e2e-aws

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@alfonsomthd
Copy link
Collaborator

@TimothyAsirJeyasing After some investigation about the E2E errors I found that the chart was crashing (at least when you create a storage system and there is no total/available/used capacity yet).
The cause was the tooltip that depends on victory-tooltip dep.
Adding this under the resolutions section of package.json solved the issue in my environment:
"@patternfly/react-charts/victory-tooltip": "^37.1.1"
Probably you'll need to also add this in the shared library.
Please test that all charts affected by this PR work as expected.
@SanjalKatiyar FYI.

@SanjalKatiyar
Copy link
Collaborator

SanjalKatiyar commented Sep 6, 2024 •
edited
Loading

If version upgrade is causing multiple issues, can't we try to fix tooltip based on some CSS property ?? (like z-index or something similar)
Let's drop the version upgrade for now, there is no other need to upgrade.

@TimothyAsirJeyasing
Copy link
Contributor Author

The react-charts version updated in #1650

@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@alfonsomthd
Copy link
Collaborator

The react-charts version updated in #1650

@SanjalKatiyar this PR can be closed once @TimothyAsirJeyasing confirms that the issue is fixed in master.
The bug should point at #1650 as its fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alfonsomthd alfonsomthd alfonsomthd left review comments

Assignees
No one assigned
Labels
needs-rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@TimothyAsirJeyasing @GowthamShanmugam @SanjalKatiyar @alfonsomthd @openshift-merge-robot