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

Enhance controller templates docs #300

Merged

Conversation

pinzonjulian
Copy link
Contributor

@pinzonjulian pinzonjulian commented Sep 8, 2020

Enhancement

Description

This PR enhances the comments included in the application_controller.js.tt and %file_name%_controller.js.tt that explain how stimulus controllers work in the context of StimulusReflex.

Fixes #298

Why should this be added

This PR solves a few doubts and gotchas I experienced myself as a first time user of SR. By enhancing these comments, I expect new developers using the gem to encounter less unexpected errors and be able to use the gem out of the box with less problems.

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing

@pinzonjulian pinzonjulian marked this pull request as draft September 8, 2020 02:01
@pinzonjulian
Copy link
Contributor Author

@leastbad I didn't quite understand what you meant by:

in the application controller template, re-word so that it's more clear and explicit: "these are event handlers for every reflex, for every controller that inherits from it"

By event handlers do you mean the lifecycle methods/callbacks?

@pinzonjulian pinzonjulian force-pushed the enhance_controller_templates_docs branch 4 times, most recently from 310682e to e0fe347 Compare September 8, 2020 02:36
@pinzonjulian pinzonjulian force-pushed the enhance_controller_templates_docs branch from e0fe347 to b8c5e97 Compare September 8, 2020 02:43
@pinzonjulian
Copy link
Contributor Author

Hey @leastbad

I have a failing test I couldn't get how to fix.
https://github.com/hopsoft/stimulus_reflex/pull/300/checks?check_run_id=1083901859

It's comparing the output of the generated file to something but I'm not sure what's it comparing it to.

These are the test lines responsible:
assert_file "app/javascript/controllers/demo_controller.js", /DemoReflex/
assert_file "app/javascript/controllers/posts_controller.js", /PostsReflex/

Could you guide through it? I haven't written these kinds of tests before.

@marcoroth
Copy link
Member

Hey @pinzonjulian!

Thank you for the PR! This is looking good so far.

By event handlers do you mean the lifecycle methods/callbacks?

Yes, he is talking about the four lifecycle methods in the ApplicationController template. (beforeReflex, reflexSuccess, reflexError and afterReflex).

See:
https://github.com/hopsoft/stimulus_reflex/blob/b8c5e973ea6b8186d76ec117dda96b0d9427a2d7/lib/generators/templates/app/javascript/controllers/application_controller.js.tt#L34-L44

Concerning the tests:

assert_file is checking if a given file is present and in this case it is also checking if the word DemoReflex is present in that file. Since you changed it to just the name without the Reflex you should be able to change the test to something like:

assert_file "app/javascript/controllers/demo_controller.js", /Demo/

Maybe it also make sense to check if the action is also present since Demo could also be pretty ambiguous.

I hope this helps! 😊

@leastbad
Copy link
Contributor

leastbad commented Sep 8, 2020

I believe reflexHalted is a thing, now, too!

@leastbad leastbad marked this pull request as ready for review September 18, 2020 09:34
@leastbad leastbad merged commit 2fc1156 into stimulusreflex:master Sep 18, 2020
@leastbad
Copy link
Contributor

We can add more to this, later! For now, these changes are great. Thanks @pinzonjulian!

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.

Update client controller template comments
3 participants