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

extract create modal actions #1516

Merged
merged 23 commits into from
Mar 1, 2022

Conversation

wychoong
Copy link
Contributor

@wychoong wychoong commented Feb 9, 2022

No description provided.

@danharrin
Copy link
Member

Method names like getCancelModalAction() aren't appropriate for traits like this, as there are many cancel actions on the class. Prefix each method name with getCreateAction, so getCreateActionCancelModalAction() or similar. Also, do you need to do the same for CanAttachRecords?

@danharrin danharrin marked this pull request as draft February 11, 2022 19:52
@wychoong
Copy link
Contributor Author

Method names like getCancelModalAction() aren't appropriate for traits like this, as there are many cancel actions on the class. Prefix each method name with getCreateAction, so getCreateActionCancelModalAction() or similar. Also, do you need to do the same for CanAttachRecords?

Will look into both.

Actually the purpose of extracting is I want to remove createAndCreateAnother. Do you think it would be better to:

  • a config to disable createAndCreateAnother globally
  • Add prepend/append form actions method (if not available yet)

@danharrin
Copy link
Member

How about config('filament.resources.have_create_and_create_another_actions')?

@MACscr
Copy link
Contributor

MACscr commented Feb 15, 2022

global options are great, but i would also like to be able to set this per resource as well.

@wychoong
Copy link
Contributor Author

Prefix each method name with getCreateAction,

this will result to

$this->getCreateActionCreateModalAction(),
$this->getCreateActionCreateAndCreateAnotherModalAction(),
$this->getCreateActionCancelModalAction(),

is that ok?

@wychoong wychoong marked this pull request as ready for review February 21, 2022 10:48
@danharrin
Copy link
Member

Prefix each method name with getCreateAction,

this will result to

$this->getCreateActionCreateModalAction(),
$this->getCreateActionCreateAndCreateAnotherModalAction(),
$this->getCreateActionCancelModalAction(),

is that ok?

Yup that's fine

@wychoong
Copy link
Contributor Author

Prefix each method name with getCreateAction,

this will result to

$this->getCreateActionCreateModalAction(),
$this->getCreateActionCreateAndCreateAnotherModalAction(),
$this->getCreateActionCancelModalAction(),

is that ok?

Yup that's fine

the PR is ready, it's now is more than just extracting modal actions 😅 but it's to make resource actions consistent with RM.

one part I'm not sure is it the best to set the static property globally with FilamentManager and RelationManager

@danharrin
Copy link
Member

Thanks. I have simplified this a bit, mostly by removing the facade option. I've also moved a couple of the static methods onto the trait instead of the relation manager, which means you need to disable these actions for each type of relation manager instead of all at once. This just helps us keep the maintenance overhead for this feature low.

@danharrin danharrin merged commit 541d7aa into filamentphp:2.x Mar 1, 2022
@wychoong wychoong deleted the extract-create-modal-actions branch March 2, 2022 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants