-
Notifications
You must be signed in to change notification settings - Fork 4
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
Refactor: {{action}}
helper usage, the sequel
#896
base: refactor/action_helper_usage
Are you sure you want to change the base?
Refactor: {{action}}
helper usage, the sequel
#896
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor/action_helper_usage #896 +/- ##
================================================================
- Coverage 13.63% 13.32% -0.31%
================================================================
Files 450 451 +1
Lines 3081 3076 -5
================================================================
- Hits 420 410 -10
- Misses 2661 2666 +5 ☔ View full report in Codecov by Sentry. |
…er_usage_electric_boogaloo
8ab55a1
to
4ed2b17
Compare
I initially began rewriting components to glimmer when I encountered them, but that's quite a lot of work and not strictly related to the task this PR is really meant for. I've found it more efficient to just refactor all action helpers and leave other non-octane things as they are for now. We can tackle them in other PRs. I've also not tested that every action occurrence I've refactored in in this PR still functions properly. Not yet at least. I did test that for #635 , but I figured it's more efficient to first rewrite, and then go through all the corresponding pages and test their functioning. |
…er_usage_electric_boogaloo
DO NOT MERGE BEFORE #635 IS MERGED
bases on #635
ref #899
may require a third PR after this
How to review this:
New prevent-default helper
This PR also introduces a new template helper:
prevent-default
. It is used to wrap the action we assign to be executed when an event takes place. Specifically, it adds a call toevent.preventDefault()
. Why would we do this in a helper? Well:e.preventDefault();
calls.preventDefault
should take place in the template, because that's where it is easiest to determine.To elaborate on point two, when we have the following action:
Then a template with
requires a call to
e.preventDefault()
:otherwise the page is reloaded automatically (because that's what a submit event on a form does, apparently.) Now, suppose instead the template contains
then we don't need a
e.preventDefault()
call... See how that's confusing? Sidenote: in reality the underlying ComponentForDoingSomething generally still performs a call toe.preventDefault
, but that's none of our business, since principles like dependency inversion dictate that we do not concern ourselves with the implementation details of a child component, but only with the parts we can 'see' of that child component.Now let's make the point even better: if we have a problematic template that contains both of these types of usages:
then what do we do? We need to call
e.preventDefault()
for one element in the template, but not for the other. But that's easily solved with the new helper: