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

Fix item action guide #500

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Fix item action guide #500

wants to merge 3 commits into from

Conversation

krns
Copy link
Member

@krns krns commented Jul 31, 2024

No description provided.

@krns krns requested a review from Flo0807 July 31, 2024 18:51
@@ -151,6 +153,9 @@ defmodule DemoWeb.ItemAction.SoftDelete do
@impl Backpex.ItemAction
def label(_assigns), do: Backpex.translate("Delete")

@impl Backpex.ItemAction
def confirm(_assigns), do: "Why do you want to delete this item?"
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 confirm function is mandatory.

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 could omit the confirm_label and cancel_label

Copy link
Collaborator

Choose a reason for hiding this comment

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

The confirm/1 function determines whether a confirmation modal is shown to the user. We can decide to add the confirm/1, confirm_label/1 and cancel_label/1 functions (a modal is shown) or remove them (no modal is shown) 🤔.

If we add the confirm/1 function as suggested, we must also keep confirm_label/1 and cancel_label/1.

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 current example in the documentation makes no sense (confirm_label/1 and cancel_label/1 without modal).

If we add the confirm/1 function as suggested, we must also keep confirm_label/1 and cancel_label/1.

Thats not right. When omitted the default values ("Cancel" and "Apply") are used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did not see the default implementation 🙃

@Flo0807
Copy link
Collaborator

Flo0807 commented Aug 8, 2024

Currently, the item action confirmation dialog is enabled when the list of fields is empty or the confirm/1 function is exported (see has_modal?/1).

From our docs:

To enable the confirmation dialog you need to implement the confirm_label/1 function

This doesn't seem right and has to be updated.

Moreover, we should update the has_modal?/1 function because you always need the confirm/1 function if there is a list of fields.

@krns

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.

2 participants