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

[data grid] Remove unnecessary comment in useGridApiOptionHandler #14463

Merged
merged 4 commits into from
Sep 19, 2024
Merged

[data grid] Remove unnecessary comment in useGridApiOptionHandler #14463

merged 4 commits into from
Sep 19, 2024

Conversation

k-rajat19
Copy link
Contributor

doing this will ensure that we are not registering multiple event handlers with the same event name :)

@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Sep 3, 2024
romgrk
romgrk reviewed Sep 3, 2024
View reviewed changes
Copy link
Contributor

@romgrk romgrk left a comment

Choose a reason for hiding this comment

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

Thanks for the work, but I don't think we should merge this :| I don't see a need to add validation code if it's not catching an actual problem.

@k-rajat19
Copy link
Contributor Author

k-rajat19 commented Sep 3, 2024 •
edited
Loading

This will help prevent any future regressions.
Currently, we are not registering multiple event handlers with the same event name anywhere in the codebase but in case we do so somewhere deep in the codebase and forget that we have already registered a handler for that event name, it will prevent that.
Also, we are exporting useGridApiOptionHandler so if users try to do the same it will prevent them too.

these are my Points of view. however, you know better than me, Feel free to close this if you want :)

@k-rajat19 k-rajat19 requested a review from romgrk September 13, 2024 11:29
@romgrk
Copy link
Contributor

romgrk commented Sep 18, 2024

I'd rather close it, we have tests for all new features so I doubt we wouldn't catch an issue like this.

@romgrk romgrk closed this Sep 18, 2024
k-rajat19
k-rajat19 commented Sep 18, 2024
View reviewed changes
packages/x-data-grid/src/hooks/utils/useGridApiEventHandler.ts
export function useGridApiOptionHandler<Api extends GridApiCommon, E extends GridEvents>(
apiRef: React.MutableRefObject<Api>,
eventName: E,
handler?: GridEventListener<E>,
) {
// Validate that only one per event name?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@romgrk Does it make sense to remove this comment, which motivates me to add these changes 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that's an old comment. If you want you can update this PR directly, I'll re-open & merge.

Copy link
Contributor Author

@k-rajat19 k-rajat19 Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I have pushed a new commit in this PR, could you re-open this PR?

@k-rajat19 k-rajat19 changed the title [data grid] add validation in useGridApiOptionHandler [data grid] remove unnecessary comment in useGridApiOptionHandler Sep 18, 2024
@romgrk romgrk reopened this Sep 18, 2024
@romgrk romgrk enabled auto-merge (squash) September 18, 2024 10:38
@mui-bot
Copy link

mui-bot commented Sep 18, 2024 •
edited
Loading

Deploy preview: https://deploy-preview-14463--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 97e4c10

auto-merge was automatically disabled September 19, 2024 09:08

Head branch was pushed to by a user without write access

@romgrk romgrk merged commit 629612e into mui:master Sep 19, 2024
18 checks passed
@k-rajat19 k-rajat19 deleted the gridOptionHandler branch September 19, 2024 10:32
arthurbalduini pushed a commit to arthurbalduini/mui-x that referenced this pull request Sep 30, 2024
@k-rajat19
[data grid] remove unnecessary comment in useGridApiOptionHandler (m…
0533453 
…ui#14463)
@oliviertassinari oliviertassinari changed the title [data grid] remove unnecessary comment in useGridApiOptionHandler [data grid] Remove unnecessary comment in useGridApiOptionHandler Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romgrk romgrk romgrk approved these changes

Assignees
No one assigned
Labels
component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@k-rajat19 @romgrk @mui-bot @zannager