Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Refactor: {{action}} helper usage, the sequel #896

Merged
merged 68 commits into from
Mar 9, 2025

Conversation

DrumsnChocolate
Copy link
Contributor

@DrumsnChocolate DrumsnChocolate commented Nov 3, 2024

DO NOT MERGE BEFORE #635 IS MERGED

bases on #635
ref #899
may require a third PR after this

How to review this:

  • have a look at some of the changes, to get an idea of what this PR is doing.
  • manually test all pages that have been affected by this PR, and see if the buttons on those pages work as expected.

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 to event.preventDefault(). Why would we do this in a helper? Well:

  1. because we don't want to pollute the js files with e.preventDefault(); calls.
  2. the decision of whether or not to call 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:

@action
someAction() {
    someLogic();
}

Then a template with

<form {{on 'submit' someAction}}></form>

requires a call to e.preventDefault():

@action
someAction(event) {
    event.preventDefault();
    someLogic();
}

otherwise the page is reloaded automatically (because that's what a submit event on a form does, apparently.) Now, suppose instead the template contains

<ComponentForDoingSomething @doThis={{someAction}}></ComponentForDoingSomething>

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 to e.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:

<form {{on 'submit' someAction}}></form>
<ComponentForDoingSomething @doThis={{someAction}}></ComponentForDoingSomething>

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:

<form {{on 'submit' (prevent-default someAction)}}></form>
<ComponentForDoingSomething @doThis={{someAction}}></ComponentForDoingSomething>

… how to do this, and it should not have taken this long lol.
…oller, and they are now all in octane style! :)
…ting late, so I will continue this some other time.
…at call the cancel action from one of the Created, Edit, Destroy Controllers
…osts in a paged fashion everytime the queryparams change
@DrumsnChocolate
Copy link
Contributor Author

DrumsnChocolate commented Nov 9, 2024

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.

@DrumsnChocolate
Copy link
Contributor Author

also fixes some styling in the print-enrolled component.
before:
afbeelding

after:
afbeelding
not perfect but quick win

@DrumsnChocolate DrumsnChocolate marked this pull request as ready for review November 9, 2024 15:38
github-merge-queue bot pushed a commit that referenced this pull request Feb 5, 2025
* todos

* add checks for overrides of ModelSaveUtils, using the entity property of ModelSaveUtils

* got the activity destroy controller working properly, it seems :)

* article comments now properly redirect. Took me a while to figure out how to do this, and it should not have taken this long lol.

* this is starting to look like something. not perfect, but it works for now

* this should comprise the last of controllers that extend DestroyController, and they are now all in octane style! :)

* debit collection show is currently broken, do not understand why. getting late, so I will continue this some other time.

* starting to get a grip on how ember works, I think

* more progress, now also added cancel action to editcontroller, and refactoring templates to use the new action

* refactoring all usages of "Annuleren" LinkTo elements into buttons that call the cancel action from one of the Created, Edit, Destroy Controllers

* jslint

* FINALLY figured out how to do a union of ObjectProxies

* moved thread save logic to model

* more stuff

* fix debit collection controllers

* going through the controllers alphabetically, decided to add some method to the madness lol

* idk, changes from last time I worked on this

* fix js lint

* fix template lint

* I found a way to keep the model just the model, while also querying posts in a paged fashion everytime the queryparams change

* more changes

* more changes

* typo

* better handling of transitions in destroycontrollers

* I think I have had all new edit and destroy controllers

* js lint fixes

* remove comment

* start of refactoring action helper usage

* remove preventdefaults

* rewrite board room presence in octane

* use @action syntax on newPresence, and move permission check to template

* use on, fn

* modal, sidebar

* rewrite privacy-modal to octane (afaik)

* remove unused import

* action helper in public-activity-card-small

* write open and closed question components in octane

* fix lint

* fix broken submit based on #896

* fix poll form
Base automatically changed from refactor/action_helper_usage to staging February 5, 2025 20:09
Copy link
Contributor

@lodewiges lodewiges left a comment

Choose a reason for hiding this comment

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

LGTM

i think you forgot 4 files
app/components/advertisement-tool.js
app/components/boolean-tag.js
app/components/cards/activity-card.js
app/components/cards/photo-album-card.js
when these are updated i will aprove

edit: i am stupid, ember/component is not the same

@lodewiges lodewiges added this pull request to the merge queue Mar 9, 2025
Merged via the queue into staging with commit ecd9918 Mar 9, 2025
3 of 5 checks passed
@lodewiges lodewiges deleted the refactor/action_helper_usage_electric_boogaloo branch March 9, 2025 22:54
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants