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

Partially revert #2279 CreateItemModal props #2462

Closed
wants to merge 2 commits into from

Conversation

gautamsi
Copy link
Member

@gautamsi gautamsi commented Mar 2, 2020

I really feel that the modals should be independent from useList variables.

in #2279 CreateItemModal was made dependent directly on useList() variables which prevents me from extending the admin pages and make use of multiple CreateItemModal component (custom actions for duplicate).

I reverted that specific dependency and made is dependent to useList() state via props, this way I can use multiple CreateItemModal without making big change to the admin ui.

@changeset-bot
Copy link

changeset-bot bot commented Mar 2, 2020

🦋 Changeset is good to go

Latest commit: 77399be

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@MadeByMike
Copy link
Contributor

This isn't intended to be a general purpose modal component. It should only be used for creating items.

The overall intention is that the admin UI components is can be used without any props. This is done with a bunch of providers. The list provider tells the createItemModal which list should be used for creating the item. If you are already in a list context this 'just works' but if you need you can wrap import the list provider and wrap the button with a new context.

See the relationship field for an example of this.

@MadeByMike MadeByMike closed this Mar 3, 2020
@gautamsi
Copy link
Member Author

gautamsi commented Mar 3, 2020

There is a use case with duplicate item action (pending for long) which could reuse the modal.

@MadeByMike
Copy link
Contributor

Wrap it in another provider and it works

@MadeByMike
Copy link
Contributor

If there is a case where this doesn't work please post me an isolated example to reproduce and I'll happily help find a solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants