feature: action dynamic backdrop option

[thiagoyoussef]

Description

Fixes #3326
Related PR: avo-hq/docs.avohq.io#303

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works

Screenshots & recording

https://www.loom.com/share/970418f8279244f7b4feb97e2617bff5?sid=f44ba85f-55c0-4809-a464-cc1328f628a2

Manual review steps

By default, action modals use a dynamic backdrop. Added a self.dynamic_backdrop = false action option in case you want to prevent the user from closing the modal when clicking on the backdrop.

class Avo::Actions::DummyAction < Avo::BaseAction
  self.name = "Dummy action"
  self.dynamic_backdrop = false

  def handle(query:, fields:, current_user:, resource:, **args)
    # Do something here
    succeed 'Yup'
  end
end

Questions

I saw that other action options like no_confirmation and standalone doesn't have tests.

1. Should I add tests for a simple feature like this one?

PS: It's not laziness, I'm assuming it's not worth it in this context.

---

I left a usage example on the Avo::Actions::ExportCsv action:

class Avo::Actions::ExportCsv < Avo::BaseAction
  ...
  self.dynamic_backdrop = false
  ...
end

2. Should I remove it?

---

I decided to use the name dynamic_backdrop instead of ⁠close_on_backdrop_click. Usually dynamic/static is used to indicate the backdrop behavior.
3) Do you prefer to keep it ⁠close_on_backdrop_click ?

---

4. Should I add dynamic_backdrop option on the action generator?

[codeclimate]

Code Climate has analyzed commit a565cba and detected 0 issues on this pull request.

View more on Code Climate.

[Paul-Bob]

Thank you for this contribution @thiagoyoussef!

I'll address all your bullet point questions and let a code review.

I saw that other action options like no_confirmation and standalone doesn't have tests.
Should I add tests for a simple feature like this one?
Yes, tests are crucial. When we identify missing tests, we normally implement them.

The no_confirmation option is tested in this test case. In this scenario, invoking the action triggers the expected response without requiring any user confirmation.

The standalone option is also covered. For actions marked as standalone, no record selection is necessary to execute them. This behavior is validated across several test cases, specifically those that run Dummy action without selecting any records.

Lastly, if a specific option name doesn't appear in the test titles, it doesn't necessarily mean it's untested. Many options are implicitly covered by existing test cases, even if not directly named.

I left a usage example on the Avo::Actions::ExportCsv action:
Should I remove it?

No, it's ok, we can use that action to test this option.

I decided to use the name dynamic_backdrop instead of ⁠close_on_backdrop_click. Usually dynamic/static is used to indicate the backdrop behavior.
3) Do you prefer to keep it ⁠close_on_backdrop_click ?

Got it, naming is sensible, I like dynamic_backdrop but close_on_backdrop_click seems more expressive, anyway dynamic_backdrop sounds good, do you have any other suggestions? @adrianthedev WDYT about naming here?

Let’s keep this option name for now and revisit it after gathering more feedback.

Should I add dynamic_backdrop option on the action generator?

No, it's all good. We considered adding more options to the templates, but it can quickly lead to cluttered configuration in each generated file. This can feel overwhelming, especially for new users who just want to create a simple action when starting with Avo. Let's focus on ensuring that this option is documented and easy to find.

Thank you again for this contribution @thiagoyoussef, I also left some code review with a duplication removal suggestion and a question about whether the new backdrop target is necessary.

[Paul-Bob]

Remove duplicated

Suggested change

	data-modal-target="modal"
	data-modal-dynamic-backdrop-value="<%= dynamic_backdrop %>"
	data-modal-dynamic-backdrop-value="<%= dynamic_backdrop %>"

[Paul-Bob]

Why do we need this check?: event.target === this.backdropTarget

Wouldn't this work?

Suggested change

	if (event.target === this.backdropTarget && !this.dynamicBackdropValue) return
	if (!this.dynamicBackdropValue) return

[thiagoyoussef]

We need to check if the event comes from the backdrop. If we don't, it would also prevent the user from closing the modal on the cancel button.

[adrianthedev]

How about self.click_backdrop_to_close_modal?

[thiagoyoussef]

@Paul-Bob thank you for the code review! Will make the changes 😄

[thiagoyoussef]

How about self.click_backdrop_to_close_modal?

Great, it's very clear!

Do I change it to click_backdrop_to_close_modal then?

Maybe close_modal_on_backdrop_click?

Should I keep using dynamic_backdrop on the modal component prop?

[Paul-Bob]

Great, it's very clear!
Do I change it to click_backdrop_to_close_modal then?
Maybe close_modal_on_backdrop_click?

close_modal_on_backdrop_click is a good suggestion, let's apply it.

Should I keep using dynamic_backdrop on the modal component prop?

Let's stick to one name for this option across all files and components. This will make it easier to find and manage, like when you're doing a quick global search to see all the places where it's used.

[Paul-Bob]

Thank you @thiagoyoussef!