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

[data grid] Add Api type param to cell params model #15968

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

k-rajat19
Copy link
Contributor

Closes #15963

@k-rajat19 k-rajat19 changed the title [data grid] Add Api type param to cell params interfaces [data grid] Add Api type param to cell params model Dec 21, 2024
@mui-bot
Copy link

mui-bot commented Dec 21, 2024

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

Generated by 🚫 dangerJS against 52905bc

@k-rajat19
Copy link
Contributor Author

k-rajat19 commented Dec 27, 2024

@michelengelen, could you review this?
Side Note: I'm not reexporting these types for pro and premium packages separately, one can pass pro and premium grid API in Api type param easily.

Copy link
Member

@michelengelen michelengelen left a comment

Choose a reason for hiding this comment

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

LGTM, but I would like a second review from @arminmeh about this.

@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Dec 30, 2024
@arminmeh
Copy link
Contributor

arminmeh commented Jan 3, 2025

Side Note: I'm not reexporting these types for pro and premium packages separately, one can pass pro and premium grid API in Api type param easily.

Is this expected from the users?

Looking backwards, I think that we shouldn't have done this
#14201 (related issue #14190)

This made api public API

We are not accessing api in any of our examples. People can use internal API, but that does not mean that we have to make it public.

@mui/xgrid should we remove api from the public API in v8 and just patch v7 to support proper types?

@cherniavskii
Copy link
Member

This made api public API
We are not accessing api in any of our examples. People can use internal API, but that does not mean that we have to make it public.

Not sure what you mean here. api in this case is the same as apiRef.current, isn't it?

@arminmeh
Copy link
Contributor

arminmeh commented Jan 6, 2025

api in this case is the same as apiRef.current, isn't it

true, but we added another public point of accessing it.
I didn't deep dive into the initial issue where it was requested to document this, but I feel that we didn't had to.
Then we would also not have this "problem". And the proposed solution is that the developers still have to do some additional work, so it is just creating unnecessary complexity IMO

@arminmeh
Copy link
Contributor

arminmeh commented Jan 6, 2025

api was attached to the cell params because of https://github.com/mui/mui-x/blob/v7.23.5/packages/x-data-grid/src/components/cell/GridCell.tsx#L196
I don't think it was meant to be documented and part of the public API

@romgrk
Copy link
Contributor

romgrk commented Jan 6, 2025

I think we should be removing api from the public API. We also expose our API as api is other places, that we should also remove.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
7 participants