8000 [BUGFIX] Table: fix reorder columns button tooltip by andreasgerstmayr · Pull Request #2722 · perses/perses · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[BUGFIX] Table: fix reorder columns button tooltip #2722

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
Merged
merged 2 commits into from
Mar 10, 2025
Merged

[BUGFIX] Table: fix reorder columns button tooltip #2722

merged 2 commits into from
Mar 10, 2025

Conversation

andreasgerstmayr
Copy link
Contributor

Description

The children component of a <Tooltip> must accept a ref and spread the passed props:
https://mui.com/material-ui/react-tooltip/#custom-child-element

Fixes the following error in the JS console when editing a Table Panel and opening the "Column Settings" tab:

ColumnEditorContainer.tsx:91 Warning: Failed prop type: Invalid prop `children` supplied to `ForwardRef(Tooltip)`. Expected an element that can hold a ref. Did you accidentally use a plain function component for an element instead? For more information see https://mui.com/r/caveat-with-refs-guide
    at Tooltip (http://localhost:3000/plugins/Table/static/js/async/vendors-node_modules_mui_material_Button_Button_js-node_modules_mui_material_Divider_Divider_-a0c33b.js:14539:97)
    at ColumnEditorContainer (http://localhost:3000/plugins/Table/static/js/async/__federation_expose_Table.js:1387:11)

Screenshots

no UI changes

Checklist

  • Pull request has a descriptive title and context useful to a reviewer.
  • Pull request title follows the [<catalog_entry>] <commit message> naming convention using one of the
    following catalog_entry values: FEATURE, ENHANCEMENT, BUGFIX, BREAKINGCHANGE, DOC,IGNORE.
  • All commits have DCO signoffs.

UI Changes

  • Changes that impact the UI include screenshots and/or screencasts of the relevant changes.
  • Code follows the UI guidelines.
  • Visual tests are stable and unlikely to be flaky.
    See Storybook
    and e2e docs for more details. Common issues
    include:
    • Is the data inconsistent? You need to mock API requests.
    • Does the time change? You need to use consistent time values or mock time utilities.
    • Does it have loading states? You need to wait for loading to complete.

@andreasgerstmayr
[BUGFIX] Table: fix reorder columns button tooltip
bb9826f 
The children component of a <Tooltip> must accept a ref and spread the
passed props:
https://mui.com/material-ui/react-tooltip/#custom-child-element

Fixes the following error in the JS console when editing a Table Panel
and opening the "Column Settings" tab:
```
ColumnEditorContainer.tsx:91 Warning: Failed prop type: Invalid prop `children` supplied to `ForwardRef(Tooltip)`. Expected an element that can hold a ref. Did you accidentally use a plain function component for an element instead? For more information see https://mui.com/r/caveat-with-refs-guide
    at Tooltip (http://localhost:3000/plugins/Table/static/js/async/vendors-node_modules_mui_material_Button_Button_js-node_modules_mui_material_Divider_Divider_-a0c33b.js:14539:97)
    at ColumnEditorContainer (http://localhost:3000/plugins/Table/static/js/async/__federation_expose_Table.js:1387:11)
```

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
@Nexucis
Copy link
Member
Nexucis commented Mar 7, 2025

@AntoineThebaud the CI is failing here because (I think) when the PR is coming from a fork, github-action does not inject any secret (for security matter)

@Nexucis
Copy link
Member
Nexucis commented Mar 7, 2025

Perhaps that's a use case that should be shared with the Cuelang team. We cannot import the Cuelang dependencies in a CI/CD as it require a token to get them and when the PR is coming from a fork, the token cannot be used.

@AntoineThebaud
Copy link
Contributor

Perhaps that's a use case that should be shared with the Cuelang team. We cannot import the Cuelang dependencies in a CI/CD as it require a token to get them and when the PR is coming from a fork, the token cannot be used.

I asked on CUE's slack about this, let's see..

@AntoineThebaud
Copy link
Contributor
AntoineThebaud commented Mar 7, 2025 •
edited
Loading

Perhaps that's a use case that should be shared with the Cuelang team. We cannot import the Cuelang dependencies in a CI/CD as it require a token to get them and when the PR is coming from a fork, the token cannot be used.

I asked on CUE's slack about this, let's see..

This is what they replied:

We plan on supporting OIDC Connect in Github Actions, which I think would solve this use case. For now, I'm afraid that people will probably have to provide their own secrets, otherwise there's a clear security risk: anyone contributing to a fork could steal the token because they control the action script.

-> I wonder if there is a way to avoid requesting every contributor to set up their own token. Like, could we isolate these tests in a dedicated job workflow/job that would somehow require a maintainer's approval to be triggered?

EDIT Other input provided since regarding short-term workaround:

An alternative for now, which we use in the main CUE repo, is to use the https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request_target event.
Note this has security implications, so please be aware of these if you go down this path.

@Nexucis
Copy link
Member
Nexucis commented Mar 10, 2025

I have isolated the tests requiring the cuelang dependency manager with the PR #2729.

@andreasgerstmayr can you update your PR with main to see if the CI is green again please ?

Nexucis
Nexucis approved these changes Mar 10, 2025
View reviewed changes
Copy link
Member
@Nexucis Nexucis left a comment

Choose a reason for hiding this comment

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

otherwise, once the CI is green, approved !

@andreasgerstmayr andreasgerstmayr added this pull request to the merge queue Mar 10, 2025
@andreasgerstmayr
Copy link
Contributor Author

@andreasgerstmayr can you update your PR with main to see if the CI is green again please ?

done
CI is green now, thanks!

I pressed the merge button :)

Merged via the queue into perses:main with commit 5f0f8c2 Mar 10, 2025
19 checks passed
@andreasgerstmayr andreasgerstmayr deleted the fix-table-reorder-columns-button branch March 10, 2025 16:28
jgbernalp pushed a commit that referenced this pull request Mar 13, 2025
@andreasgerstmayr @jgbernalp
[BUGFIX] Table: fix reorder columns button tooltip (#2722)
3311b59 
The children component of a <Tooltip> must accept a ref and spread the
passed props:
https://mui.com/material-ui/react-tooltip/#custom-child-element

Fixes the following error in the JS console when editing a Table Panel
and opening the "Column Settings" tab:
```
ColumnEditorContainer.tsx:91 Warning: Failed prop type: Invalid prop `children` supplied to `ForwardRef(Tooltip)`. Expected an element that can hold a ref. Did you accidentally use a plain function component for an element instead? For more information see https://mui.com/r/caveat-with-refs-guide
    at Tooltip (http://localhost:3000/plugins/Table/static/js/async/vendors-node_modules_mui_material_Button_Button_js-node_modules_mui_material_Divider_Divider_-a0c33b.js:14539:97)
    at ColumnEditorContainer (http://localhost:3000/plugins/Table/static/js/async/__federation_expose_Table.js:1387:11)
```

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nexucis Nexucis Nexucis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andreasgerstmayr @Nexucis @AntoineThebaud
0