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

CRUD component #4486

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

CRUD component #4486

wants to merge 77 commits into from

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Nov 27, 2024

Implement CRUD feature as specified in #4146

https://deploy-preview-4486--mui-toolpad-docs.netlify.app/toolpad/core/react-crud/

Tests and examples will be added in separate PRs as this PR has already grown too large.

To help review, the contents of this PR are mostly:

  • Relevant changes for the new feature (new components) in packages/toolpad-core.
  • Documentation added in docs, many examples but with a lot of repetition between them, feel free to just mostly take a look at the new documentation page for the Crud component.
  • Added orders CRUD to the playgrounds for Vite, Next.js App Router and Next.js Pages Router.
  • Other than that there are very few changes.

It took a while to reach this first version but still feel free to provide any major or minor feedback!

@apedroferreira apedroferreira added new feature New feature or request scope: toolpad-core Abbreviated to "core" labels Nov 27, 2024
@apedroferreira apedroferreira self-assigned this Nov 27, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 27, 2024
@apedroferreira apedroferreira changed the title CRUD List component CRUD component Dec 24, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 22, 2025
@mui-bot
Copy link

mui-bot commented Jan 22, 2025

Netlify deploy preview

https://deploy-preview-4486--mui-toolpad-docs.netlify.app/

Generated by 🚫 dangerJS against 1e21054

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 29, 2025
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 18, 2025
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 21, 2025
@apedroferreira apedroferreira marked this pull request as ready for review February 21, 2025 14:10
@apedroferreira apedroferreira requested a review from a team February 21, 2025 14:10
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 21, 2025

const { fields, createOne, validate } = cachedDataSource;

const [formState, setFormState] = React.useState<{
Copy link
Member

@bharatkashyap bharatkashyap Feb 24, 2025

Choose a reason for hiding this comment

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

The formState calculation seems quite expensive to be made every render, here's a performance suggestion (including removing the .filter.map chain and using a single .reduce):

const createInitialValues = React.useCallback((fields, initialValues) => {
  const baseValues = fields.reduce((acc, { field, type }) => {
    if (field === 'id') return acc;
    acc[field] = type === 'boolean' ? (initialValues[field] ?? false) : initialValues[field];
    return acc;
  }, {} as Partial<OmitId<D>>);

  return {
    values: { ...baseValues, ...initialValues },
    errors: {} as Partial<Record<keyof D, string>>
  };
}, []);

const [formState, setFormState] = React.useState(() => 
  createInitialValues(fields, initialValues)
);

Copy link
Member Author

Choose a reason for hiding this comment

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

But that's only the initial state value, it doesn't run every render.
I think it should be ok, .reduce might be more efficient but not as readable.

Copy link
Member Author

@apedroferreira apedroferreira Feb 25, 2025

Choose a reason for hiding this comment

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

hm, i just looked it up and looks like it might run every render...
I guess that just passing a function with the same result still using .map and .filter would only run once though, I'll do that if it sounds cool. (just added it)

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request scope: toolpad-core Abbreviated to "core"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants