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

[docs] Encourage DataGrid in /components/tables/ over alternatives #22637

Merged
merged 4 commits into from
Sep 21, 2020
Merged

[docs] Encourage DataGrid in /components/tables/ over alternatives #22637

merged 4 commits into from
Sep 21, 2020

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Sep 17, 2020 •
edited

Prepare addition of a live demo in the /components/tables/ page.

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation component: data grid This is the name of the generic UI component, not the React module! labels Sep 17, 2020
@oliviertassinari
Copy link
Member Author

@dtassone It seems that the TypeScript bindings of the grid are not correct, see the reported error. Any idea?

@github-actions github-actions bot mentioned this pull request Sep 17, 2020
Closed
@mui-pr-bot
Copy link

mui-pr-bot commented Sep 17, 2020 •
edited

Details of bundle changes

Generated by 🚫 dangerJS against 3e3e275

@dtassone
Copy link
Member

@dtassone It seems that the TypeScript bindings of the grid are not correct, see the reported error. Any idea?

Yes I did some changes around the typings and

@dtassone It seems that the TypeScript bindings of the grid are not correct, see the reported error. Any idea?

My bad! Fixed in mui/mui-x#298

@oliviertassinari
Copy link
Member Author

@dtassone Thanks for the fix! Let's wait for the next release of @material-ui/data-grid to update the pull request.

@dtassone
Copy link
Member

@dtassone Thanks for the fix! Let's wait for the next release of @material-ui/data-grid to update the pull request.

Releasing now

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Sep 18, 2020 •
edited

@dtassone I have updated the version to v4.0.0-alpha.2, it fails with different errors now.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Sep 18, 2020 •
edited

It seems that we should add a step in the CI to make sure the generated types are sound. Then, in a second step, once we can reproduce the fail in material-ui-x, fix it.

mbrookes
mbrookes previously requested changes Sep 18, 2020
View reviewed changes
docs/src/pages/components/tables/tables.md Outdated Show resolved Hide resolved
docs/src/pages/components/tables/tables.md Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari changed the base branch from master to next September 18, 2020 20:17
@oliviertassinari oliviertassinari changed the title [docs] Add live demo of DataGrid in /components/tables/ [docs] Encourage DataGrid in /components/tables/ over alternatives Sep 18, 2020
@oliviertassinari oliviertassinari force-pushed the one-data-grid branch 2 times, most recently from f9579a5 to 712a299 Compare September 18, 2020 20:22
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 18, 2020
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 18, 2020
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Sep 18, 2020 •
edited

@dtassone I have noticed another issue with the types of @material-ui/data-grid, they import from a node js module (events), it creates confusion for TypeScript, he thinks createTimeout returns a node's structure instead of a number like it does in the browser. We had a similar issue in https://github.com/mui-org/core-plus-pickers/pull/12.

Anyway, I have removed the live demo as we can still deliver part of the value.

eps1lon
eps1lon reviewed Sep 19, 2020
View reviewed changes
docs/next.config.js
@@ -100,33 +100,8 @@ module.exports = {
// transpile 3rd party packages with dependencies in this repository
{
test: /\.(js|mjs|jsx)$/,
include: /node_modules(\/|\\)(material-table|notistack|@material-ui(\/|\\)pickers)/,
use: {
Copy link
Member

@eps1lon eps1lon Sep 19, 2020 •
edited

Choose a reason for hiding this comment

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

Why remove the use options?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have replaced it with the default Babel loader. It's not required per say for the changes of the pull request. Happy to revert to only propose them once we have a live demo.

Copy link
Member

Choose a reason for hiding this comment

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

But why? We shouldn't fully transpile these modules since that would hide potential mismatches in transpilation targets. If external libraries would start transpiling for modern browsers by default we wouldn't ever notice.

Right now it would just add more transpilation work.

Copy link
Member Author

Choose a reason for hiding this comment

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

The change was reverted. I have removed the live demo, it wasn't ready.

Copy link
Member Author

Choose a reason for hiding this comment

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

We will be able to resume the discussion with a future pull request to add a live demo.

Copy link
Member

Choose a reason for hiding this comment

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

The change was reverted. I have removed the live demo, it wasn't ready.

Sure. Just wanted to make sure we understand each other because I asked for a "why" and you gave me a "what".

Copy link
Member Author

Choose a reason for hiding this comment

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

For the "why", I do no longer have it in context. The build was failing, I copied and pasted the configuration of the workspace. I didn't think twice, I don't remember. Happy to dig in the follow-up pull request :)

@oliviertassinari oliviertassinari merged commit 27ada08 into mui:next Sep 21, 2020
@oliviertassinari oliviertassinari deleted the one-data-grid branch September 21, 2020 08:32
oliviertassinari added a commit that referenced this pull request Sep 21, 2020
@oliviertassinari
[docs] Encourage DataGrid in /components/tables/ over alternatives (#…
c1f122b 
…22637)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eps1lon eps1lon eps1lon left review comments

@dtassone dtassone Awaiting requested review from dtassone

@mbrookes mbrookes Awaiting requested review from mbrookes

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@oliviertassinari @mui-pr-bot @dtassone @mbrookes @eps1lon