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

[core] Initial DataGrid component #3558

Merged
merged 126 commits into from
May 28, 2024
Merged

[core] Initial DataGrid component #3558

merged 126 commits into from
May 28, 2024

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented May 16, 2024

Bring in DataProvider and DataGrid from the prototype.

Supports all CRUD operations, and serverside/clientside pagination

docs preview

Janpot added 30 commits March 26, 2024 10:50
@Janpot Janpot marked this pull request as ready for review May 24, 2024 10:33
@Janpot Janpot requested a review from a team May 24, 2024 10:34
Copy link
Member

@apedroferreira apedroferreira 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!
Didn't really comment anything major, just some grammar when i went through it and some other small things.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 26, 2024
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

A bit of feedback, can we pin the panel for adding item, I scrolled and suddenly forgot that I had the panel open and wondered why the button is disabled. If we don't pin it, maybe we can make the button always enabled and scroll to the panel if clicked twice (I would go with the pin options from these two).

One more thing, maybe we can add some separator between the fields in the panel and maybe add a disabled state if the user cannot enter some field (e.g. the id is auto-generated).

When using both update and delete, it would be great if the update icons don't cover the remove icon.

Screenshot 2024-05-27 at 10 08 21

The rest looks great 👌

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

I also noticed that the API links are broken, they point to toolpad-core/api/component instead of toolpad/core/api/component. It works as expected on master.

@Janpot
Copy link
Member Author

Janpot commented May 27, 2024

A bit of feedback, can we pin the panel for adding item, I scrolled and suddenly forgot that I had the panel open and wondered why the button is disabled.

At the moment, the add row logic is built on top of an empty row in editing mode, would like to pin this row but that's a X pro feature. Potential solutions I can think of:

  1. keep using an empty draft row, but disable scrolling when this UI is active
  2. build instead as a form in a drawer that overlays the datagrid

For me, personally, 2. makes a lot of sense. For tables with lots of columns, vertical scroll in a form feels more natural than horizontal scrolling in an editable row.

One more thing, maybe we can add some separator between the fields in the panel and maybe add a disabled state if the user cannot enter some field (e.g. the id is auto-generated).

Yep, it being built as a row makes customizing this a bit awkward

When using both update and delete, it would be great if the update icons don't cover the remove icon.

The idea would be to keep the delete action available even when the row is in update mode? Basically, show 3 icons?

@mnajdova
Copy link
Member

For me, personally, 2. makes a lot of sense. For tables with lots of columns, vertical scroll in a form feels more natural than horizontal scrolling in an editable row.

Right makes sense, I was too focused on this particular use-case.

The idea would be to keep the delete action available even when the row is in update mode? Basically, show 3 icons?

I would vote for this yes. Not a hard push tough, but it seemed a bit awkward that the icon was not there.

@Janpot
Copy link
Member Author

Janpot commented May 27, 2024

Ok, I'll try it out with a form in follow-up. In the meantime I've disabled scrolling when the create flow is activated.
Another advantage would be: The current method also sort of forces us to use row editing mode only. If the creation flow does not build on top of editing, we can leave users the choice whether they want row or cell editing.

Regarding the row actions, I was trying to avoid having to make the column wider than two icons. But we can make that three. Another option is to to have in editing mode a save icon, and a menu that contains a cancel action and a delete action. If we can enable cell editing mode again, we can even reduce it to a delete icon only.

@mnajdova
Copy link
Member

Regarding the row actions, I was trying to avoid having to make the column wider than two icons. But we can make that three. Another option is to to have in editing mode a save icon, and a menu that contains a cancel action and a delete action. If we can enable cell editing mode again, we can even reduce it to a delete icon only.

Sounds good, feel free to tackle in a follow up. We can maybe ask someone from the design team for an opinion.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 27, 2024
@Janpot Janpot requested a review from apedroferreira May 27, 2024 11:20
@Janpot
Copy link
Member Author

Janpot commented May 27, 2024

I also noticed that the API links are broken, they point to toolpad-core/api/component instead of toolpad/core/api/component. It works as expected on master.

This was fixed on master in the meantime, merged these changes in this branch

@Janpot Janpot requested a review from mnajdova May 27, 2024 11:21
@apedroferreira
Copy link
Member

apedroferreira commented May 27, 2024

I also noticed that the API links are broken, they point to toolpad-core/api/component instead of toolpad/core/api/component. It works as expected on master.

I've added the fix in the monorepo mui/material-ui#42362

Ah Jan already answered!

Copy link
Member

@apedroferreira apedroferreira 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!

@Janpot Janpot merged commit 758f82c into mui:master May 28, 2024
12 checks passed
@Janpot Janpot deleted the datagrid branch May 28, 2024 09:02
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants