Skip to content

[DataGrid] Allow to customize the value displayed in the filter button tooltip #6956

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 7 commits into from
Feb 3, 2023
Merged

[DataGrid] Allow to customize the value displayed in the filter button tooltip #6956

merged 7 commits into from
Feb 3, 2023

Conversation

@ithrforu
Copy link
Contributor

@ithrforu ithrforu commented Nov 22, 2022 •
edited by m4theushw
Loading

Related to #5492 (a follow-up is to fix the built-in operators)

I faced a problem with current filter values in GridToolbarFilterButton tooltip title (use DataGridPremium) and found issue with @m4theushw comment. This problem looks like:
image
image

In this PR i added the ability (from @m4theushw proposal) to specify a function to format the filter value in GridToolbarFilterButton tooltip title.

@ithrforu ithrforu changed the title [data grid] Fix filter value in filter button tooltip [dataGrid] Fix filter value in filter button tooltip Nov 22, 2022
@mui-bot
Copy link

mui-bot commented Nov 22, 2022 •
edited
Loading

Messages
📖 Netlify deploy preview: https://deploy-preview-6956--material-ui-x.netlify.app/

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 620.3 910.5 673.6 738.48 112.75
Sort 100k rows ms 594.5 1,109.9 842.5 890.42 171.817
Select 100k rows ms 233.4 299.1 272.7 267.98 28.049
Deselect 100k rows ms 149.2 281.9 190.8 203.1 44.788

Generated by 🚫 dangerJS against 5740b4e

@zannager zannager added the scope: data grid Changes related to the data grid. label Nov 22, 2022
@ithrforu ithrforu force-pushed the filter-button-tooltip-format-value branch 3 times, most recently from 1c6c38d to 6750a5d Compare November 22, 2022 12:39
@m4theushw m4theushw changed the title [dataGrid] Fix filter value in filter button tooltip [DataGrid] Fix filter value in filter button tooltip Nov 22, 2022
@m4theushw m4theushw changed the title [DataGrid] Fix filter value in filter button tooltip [DataGrid] Allow to customize the value displayed in the filter button tooltip Nov 22, 2022
@m4theushw m4theushw changed the title [DataGrid] Allow to customize the value displayed in the filter button tooltip [DataGrid] Allow to custumize the value displayed in the filter button tooltip Nov 22, 2022
@m4theushw m4theushw changed the title [DataGrid] Allow to custumize the value displayed in the filter button tooltip [DataGrid] Allow to customize the value displayed in the filter button tooltip Nov 22, 2022
@m4theushw m4theushw added the needs cherry-pick The PR should be cherry-picked to master after merge. label Nov 22, 2022
@MBilalShafi MBilalShafi self-assigned this Dec 8, 2022
@ithrforu ithrforu force-pushed the filter-button-tooltip-format-value branch from 6750a5d to e62e81c Compare December 12, 2022 16:31
@ithrforu ithrforu requested review from MBilalShafi and removed request for cherniavskii December 12, 2022 16:37
@MBilalShafi
Copy link
Member

MBilalShafi commented Dec 19, 2022 •
edited
Loading

@ithrforu Thanks for the contribution, can we test the newly added function getValueAsString and add some example usage in docs to improve usability?

@ithrforu
Copy link
Contributor Author

ithrforu commented Dec 20, 2022

@MBilalShafi. Thank you for attention. Of course. Should I create new pull request or continue here?

@MBilalShafi
Copy link
Member

MBilalShafi commented Dec 20, 2022

Should I create new pull request or continue here

It will be better if we have it as part of this pull request so that we push this new addition tested and more usable.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Dec 21, 2022
@github-actions
Copy link

github-actions bot commented Dec 21, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@ithrforu ithrforu force-pushed the filter-button-tooltip-format-value branch from e62e81c to cc1e3bd Compare December 31, 2022 01:26
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Dec 31, 2022
@ithrforu ithrforu force-pushed the filter-button-tooltip-format-value branch 2 times, most recently from ebd8fef to ec05cb6 Compare January 1, 2023 11:28
@ithrforu
Copy link
Contributor Author

ithrforu commented Jan 1, 2023 •
edited
Loading

@MBilalShafi, I added an example and rebased with latest next. How can I test getValueAsString?

@ithrforu ithrforu force-pushed the filter-button-tooltip-format-value branch 2 times, most recently from 6ad3835 to 6fac285 Compare January 1, 2023 15:03
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jan 29, 2023
@ithrforu ithrforu force-pushed the filter-button-tooltip-format-value branch 2 times, most recently from 816534c to b7fe89a Compare January 29, 2023 18:56
@ithrforu
Copy link
Contributor Author

ithrforu commented Jan 29, 2023

@MBilalShafi, all done.

@ithrforu ithrforu force-pushed the filter-button-tooltip-format-value branch from b7fe89a to 2102b3d Compare January 29, 2023 20:54
MBilalShafi
MBilalShafi reviewed Jan 31, 2023
View reviewed changes
@ithrforu ithrforu force-pushed the filter-button-tooltip-format-value branch from 9fbc4d5 to 444f4f5 Compare January 31, 2023 17:48
@MBilalShafi MBilalShafi requested a review from m4theushw January 31, 2023 19:34
@ithrforu
Copy link
Contributor Author

ithrforu commented Feb 1, 2023

Is it bug?
MUI

m4theushw
m4theushw approved these changes Feb 1, 2023
View reviewed changes
Copy link
Collaborator

@m4theushw m4theushw 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, thanks!

@m4theushw
Copy link
Collaborator

m4theushw commented Feb 1, 2023

@ithrforu The Tooltip has a delay of 1 second to show up. I think you didn't wait enough while hovering it.

@ithrforu
Copy link
Contributor Author

ithrforu commented Feb 2, 2023 •
edited
Loading

@m4theushw, I mean I hovered filter button but "Show the source" tooltip displayed.

@LukasTy
Copy link
Member

LukasTy commented Feb 2, 2023

Indeed, this PR might have some sort of regression in hover event bubbling or something along these lines. The current versions do not display Show the full source tooltip on demo hover. 🙈

@m4theushw
Copy link
Collaborator

m4theushw commented Feb 2, 2023

@LukasTy I can also reproduce this bug in https://material-ui.netlify.app/material-ui/react-button/ so certainly is not a regression introduced by this PR but from the core.

@LukasTy
Copy link
Member

LukasTy commented Feb 2, 2023

I can also reproduce this bug in https://material-ui.netlify.app/material-ui/react-button/ so certainly is not a regression introduced by this PR but from the core.

Hm. Good point. I tried testing a local dev run and did not see this behaviour, that's why I thought that it might have something to do with this PR, but I do not see any @mui/monorepo bumps to warrant that. 🙈

@ithrforu
Copy link
Contributor Author

ithrforu commented Feb 3, 2023

Can i do something?

@MBilalShafi
Copy link
Member

MBilalShafi commented Feb 3, 2023

It's all good @ithrforu, I'll merge this now.
Regarding the above-mentioned regression, I'll create a ticket in core if there's not one already.

@MBilalShafi MBilalShafi merged commit 3e18bc0 into mui:next Feb 3, 2023
@MBilalShafi MBilalShafi mentioned this pull request Feb 3, 2023
Merged
@ithrforu ithrforu deleted the filter-button-tooltip-format-value branch February 3, 2023 13:33
@LukasTy
Copy link
Member

LukasTy commented Feb 3, 2023

It's all good @ithrforu, I'll merge this now. Regarding the above-mentioned regression, I'll create a ticket in core if there's not one already.

It might have also been a desired change from MUI Core, we just weren't aware of it. But I couldn't find a recently merged PR related to it. 🤔

@TiagoPortfolio TiagoPortfolio mentioned this pull request Mar 20, 2023
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MBilalShafi MBilalShafi MBilalShafi approved these changes

+1 more reviewer

@m4theushw m4theushw m4theushw approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@MBilalShafi MBilalShafi

Labels

needs cherry-pick The PR should be cherry-picked to master after merge. scope: data grid Changes related to the data grid.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ithrforu @mui-bot @MBilalShafi @m4theushw @LukasTy @zannager