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

feature: allow assigning Stimulus controllers to actions #2039

Merged
merged 15 commits into from
Dec 4, 2023

Conversation

bryszard
Copy link
Contributor

@bryszard bryszard commented Nov 18, 2023

Description

Fixes # 1938

Context

As explained in the linked issue, we want to be able to assign Stimulus controllers to actions, similarly as we do to the resources.

I was considering reusing the controller assigned to the resource, but I've decided that would make the feature less flexible and less understandable. Why should we be binding the JS logic on action to a given resource? What if we'd like to reuse an action for different resources?

Changes

In order to enable the feature I had to do the following:

  1. Add stimulus_controllers class attribute and get_stimulus_controllers instance method to BaseAction.
    I've considered reusing the concern HasStimulusControllers, but it would be an overfit in here because as far as I understand we don't need to add view type specific attributes.
  2. Pass action to the BaseField in order to later use it while rendering.
  3. Add Stimulus data attributes - controller to the top-level action element and target to all fields in the action form.

As an example I've reused existing Stimulus controller course_resource_controller.
I've renamed to city_in_country_controller, because it is not specific to course resource.

Out of scope

What are differences from the Stimulus controllers added for resources:

  1. I don't add default controllers per view, like we do for resource-edit, resource-show, and resource-index.
  2. I don't pass the view attribute to the DOM. The main reason is that currently standalone action is getting the view value from here and it will be always :new. To get the "real" value I believe some refactoring would be required.

Let me know if you think we should do it differently.

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

Visual QA

2023-11-19.00-39-04.mp4

Manual review steps

  1. Go to the dummy app to Courses page
  2. Check the "Show current time" action
  3. Check if the action also works on show and edit pages

Manual reviewer: please leave a comment with output from the test if that's the case.

Copy link

codeclimate bot commented Nov 18, 2023

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

View more on Code Climate.

@bryszard bryszard force-pushed the feature/assign-stimulus-to-actions branch from f910f34 to f61bbac Compare November 19, 2023 00:00
@bryszard bryszard marked this pull request as draft November 19, 2023 00:13
@bryszard
Copy link
Contributor Author

@Paul-Bob I have a problem with failing i18n_spec, but the problem does not seem to be related to my changes:

Missing translations (6) | i18n-tasks v1.0.12
+--------+------------------------------------------+-------------------------------------------------------------------------------------------+
| Locale | Key                                      | Value in other locales or source                                                          |
+--------+------------------------------------------+-------------------------------------------------------------------------------------------+
|   nb   | avo.records_selected_from_all_pages_html | en All records selected from all pages                                                    |
|   nb   | avo.x_records_selected_from_page_html    | en <span class="font-bold text-gray-700">%{selected}</span> records selected on this page |
|   nn   | avo.records_selected_from_all_pages_html | en All records selected from all pages                                                    |
|   nn   | avo.x_records_selected_from_page_html    | en <span class="font-bold text-gray-700">%{selected}</span> records selected on this page |
|   tr   | avo.records_selected_from_all_pages_html | en All records selected from all pages                                                    |
|   tr   | avo.x_records_selected_from_page_html    | en <span class="font-bold text-gray-700">%{selected}</span> records selected on this page |
+--------+------------------------------------------+-------------------------------------------------------------------------------------------+

@Paul-Bob
Copy link
Contributor

@Paul-Bob I have a problem with failing i18n_spec, but the problem does not seem to be related to my changes:

Missing translations (6) | i18n-tasks v1.0.12
+--------+------------------------------------------+-------------------------------------------------------------------------------------------+
| Locale | Key                                      | Value in other locales or source                                                          |
+--------+------------------------------------------+-------------------------------------------------------------------------------------------+
|   nb   | avo.records_selected_from_all_pages_html | en All records selected from all pages                                                    |
|   nb   | avo.x_records_selected_from_page_html    | en <span class="font-bold text-gray-700">%{selected}</span> records selected on this page |
|   nn   | avo.records_selected_from_all_pages_html | en All records selected from all pages                                                    |
|   nn   | avo.x_records_selected_from_page_html    | en <span class="font-bold text-gray-700">%{selected}</span> records selected on this page |
|   tr   | avo.records_selected_from_all_pages_html | en All records selected from all pages                                                    |
|   tr   | avo.x_records_selected_from_page_html    | en <span class="font-bold text-gray-700">%{selected}</span> records selected on this page |
+--------+------------------------------------------+-------------------------------------------------------------------------------------------+

It's not related @bryszard, don't worry. We're missing some translation on a new feature

…ead of array

In the implementation for resources we're using string. I'm not convinced that this is a right approach, but I'm applying same convention here.
@bryszard
Copy link
Contributor Author

@Paul-Bob Ok, in that case I think it's ready for review :).

@Paul-Bob Paul-Bob self-requested a review November 21, 2023 06:38
@Paul-Bob Paul-Bob changed the base branch from main to 2.x November 24, 2023 12:01
Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @bryszard ! It looks great and complete with docs and tests!

@bryszard
Copy link
Contributor Author

Thank you, Paul :). So, it will be merged only to version 2.0? Or also copied to 3.0?

@Paul-Bob
Copy link
Contributor

Thank you, Paul :). So, it will be merged only to version 2.0? Or also copied to 3.0?

Everything that is merged into 2.x is also merged into 3.x. I'll "copy" your PR and apply it to 3.x after we merge it here.

Copy link
Collaborator

@adrianthedev adrianthedev left a comment

Choose a reason for hiding this comment

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

Look good! Thanks @bryszard! This is going to improve actions so much!

@Paul-Bob Paul-Bob merged commit f0ebbf3 into avo-hq:2.x Dec 4, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Assign Stimulus controllers to actions
3 participants