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

[docs] Fix imports for x-grid-data-generator #887

Merged
merged 14 commits into from
Jan 25, 2021

Conversation

DanailH
Copy link
Member

@DanailH DanailH commented Jan 20, 2021

In response to this comment #855 (comment)

I'm not sure if the x-data-grid-generator is supposed to work with just the data-grid module. Let's discuss it here. Also if it should work then the name would also need to change IMO (remove the x- prefix).

@DanailH DanailH added core Infrastructure work going on behind the scenes discussion components: XGrid labels Jan 20, 2021
@DanailH DanailH self-assigned this Jan 20, 2021
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I'm not sure if the x-data-grid-generator is supposed to work with just the data-grid module.

OK, so we are going with option 1. sounds fair. Considering that XGrid extends DataGrid, I would expect it to work with both. We need to fix the codesandbox dependency generation logic and update all the imports as well as the peer dependency.

Also if it should work then the name would also need to change IMO (remove the x- prefix).

Regarding the naming, if we were to improve it (in the future), I would personally suggest that we create a new repository under this folder: /packages/material-ui-x. Then, we could do import {} from '@material-ui/x/dataGenerator';.

I have also noticed that we forgot to prefix the relevant folders with material-ui in https://github.com/mui-org/material-ui-x/tree/master/packages to match https://github.com/mui-org/material-ui/tree/master/packages.

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Jan 20, 2021
@oliviertassinari oliviertassinari changed the title Fix imports for x-grid-data-generator [docs] Fix imports for x-grid-data-generator Jan 20, 2021
@DanailH
Copy link
Member Author

DanailH commented Jan 20, 2021

I understand that it is still broken. I opened the PR to use it as a discussion to decide between option 1 and option 2.

@oliviertassinari oliviertassinari marked this pull request as draft January 20, 2021 16:33
:;master' of github.com:mui-org/material-ui-x into fix/DataGrid-NO_ISSUE-fix-imports
@DanailH
Copy link
Member Author

DanailH commented Jan 22, 2021

After giving it a bit more thought, removing data-grid from the dependencies looks like the way to go. I had to duplicate the v5 helpers which can be removed once we decide to fully migrate so that code is only temporary. The cell definition is needed.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 22, 2021

What's the pros of option 2 compared to option 1? I mean, what's the selling point?

(assuming this terminology #855 (comment))

@DanailH
Copy link
Member Author

DanailH commented Jan 22, 2021

Removing DataGrid from the dep would make using this simpler as you can use it with both right. Correct me if I'm wrong but if we keep it as it is and want to make it work with x-grid then you would need data-grid installed as well right, how else could you know from where to import the CellParams for example?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 22, 2021

@DanailH OK thanks. I was interested in the rationale. We trade of bit of code duplication for one less dependency in the codesandbox-es/examples repo (when using x-grid). No objections, we can give it a try.

Could we update the eslint rule to forbid imports, so we don't break the tradeoff in the future inadvertently like we did in the first place?

@dtassone
Copy link
Member

We could move it to the grid folder as well, and build it the same way we build data-grid and x-grid package.
So we remove the dependency on DataGrid and we don't duplicate code

@DanailH DanailH force-pushed the fix/DataGrid-NO_ISSUE-fix-imports branch from 22e1aa8 to 87a21ed Compare January 22, 2021 19:27
@DanailH DanailH marked this pull request as ready for review January 22, 2021 21:48
@DanailH
Copy link
Member Author

DanailH commented Jan 22, 2021

Ok, I moved the x-grid-data-generator under packages/grid and fixed the build (made it the same way it is done for the data-grid and x-grid) and removed the duplicated code.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Looks great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes discussion docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants