human-essentials icon indicating copy to clipboard operation
human-essentials copied to clipboard

Add the ability to utilize the Turbo/hotwire + StimulusJS in the application

Open edwinthinks opened this issue 1 year ago • 4 comments

Description

This PR introduces the ability to utilize turbo and/or stimulus to introduce interactivity to the application. I personally believed I've used turbo & stimulus enough to know it is a pretty solid approach to keeping things simple and more sustainable in the short & long term.

This doesn't remove any of the older ways of utilizing JS and I would have preferred to make that switch altogether. However, it isn't that easy to change everything and I hope we can migrate to using this approach.

For context -- I think this is going to be really useful for features like previewing distributions & requests without having to do anything too fancy.

Notes

  • You must disable Turbo by default since it causes issues due to making every link and interaction into turbo_stream format. To avoid cross-compatibility issues, you can selectively choose which views/controllers you want to use turbo for. In the future, it would be ideal that we can activate turbo for the whole app as it creates a "snappier" experience. Followed the guide here
  • Moves some JS features over to Stimulus & Turbo combo. Will need to identify changes we can make now and also which that should be handled in a separate issues. For example, the logic for adding/removing line items is something worth doing in a separate PR.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Tested locally using hotwire & stimulus

Screenshots

N/A

edwinthinks avatar Aug 28 '22 19:08 edwinthinks

While I like Turbo/Stimulus, I'm not sure it's worth it to introduce it as a capability if we don't have a use case for it. In my experience, if we have 2 ways of doing something inside an app and we introduce a 3rd way intending to replace the other 2, it'll only ever happen if we ruthlessly and in advance replace the existing methods. 95% of the time, devs who want to figure out the "right way" to do something will find a way it's already being done and use it as a template.

So if we want to use Stimulus, I'd try to identify at least 30-40% of the existing JS that should be using Stimulus and replace it. Otherwise we're just turning 2 confusing ways of doing things into 3 confusing ways.

dorner avatar Aug 29 '22 13:08 dorner

@dorner very good points. I agree with your assessment and would like to see us move to use Stimulus in place of Jquery and while we are at it remove JS we don't even use. I wouldn't be surprised if there is alot of them. Knowing this, do you think we ought to include moving some things to StimulusJS in this PR?

edwinthinks avatar Aug 29 '22 23:08 edwinthinks

I’d definitely like to see some replacement as part of this PR, and maybe even some additional issues to replace more JS so we don’t lose track of the goal?

dorner avatar Aug 29 '22 23:08 dorner

I’d definitely like to see some replacement as part of this PR, and maybe even some additional issues to replace more JS so we don’t lose track of the goal?

Yeh I think to get the ball rolling and opening issues would be great. I think more than that doing an assessment of what we have that can be removed. It is easier to change code that you can just delete :).

edwinthinks avatar Aug 29 '22 23:08 edwinthinks

@dorner appreciate the review. This is exciting! As we discussed, here are the issues we need to add to fully transition us to using the newer approach for JS using Turbo & Stimulus.

  • Migrating from asset pipeline require libraries to using webpacker (eventually goto esbuild when we can!)
    • quagga
    • bootstrap libraries and dependencies
    • adminlte.min
    • cocoon
    • filterrific
    • rails-ujs
  • Migrate custom JS in assets/javascript/application.js
    • Migrate or remove code that overrides the auto-hide on .alert
    • Migrate the data-partner-reminder-form logic to be a StimulusController
    • Migrate code to load the tab from the # anchor in the HTML.
    • Migrate custom imported JS files in the assets/javascripts/* folder
  • Update controllers to be turbo ready by updating the flash message to use the new approach as done in https://github.com/rubyforgood/human-essentials/commit/cff84460e99b18cab45cbb3cf6957403787ab1a1
  • Discussions
    • Should we use redis?
    • Do we need to include ActionCable to use Hotwire?

Let me know if I missed anything. I'am hoping to merge this in and create issues to take advantage of hackoctoberfest to get help here :)

edwinthinks avatar Sep 27 '22 14:09 edwinthinks

@edwinthinks is it safe to merge to main if there are all the other issues you mention? Should we just have a feature branch that we keep updated that we can merge to? I feel like JS changes a lot less often than other parts of the app so it might not be too bad to take that approach.

dorner avatar Sep 28 '22 20:09 dorner

@dorner I think it's safe based on my QA'ing and CI tests passing. The godsend is being able to restrict where the turbo is active or not. In addition, I've got some features I think we can use stimulus or hotwire for that's coming up.

edwinthinks avatar Sep 28 '22 20:09 edwinthinks

@edwinthinks How far out do you think we are on this? I ask because I'd rather not figure out how to do make the counties stuff work in the old way if this is coming down the pipe RSN, but I also want to get the counties work out to the users as soon as is feasible.

cielf avatar Oct 05 '22 16:10 cielf

Hey, @cielf I think this is pretty close. I think the last bit is to do another round of QA and then build out the issues to continue the work. We should be able to take advantage of this work soon :)!

edwinthinks avatar Oct 12 '22 00:10 edwinthinks

QA'ed and tested! I'll merge this in and will create issues to migrate more things over. I think this will be a boon to our development experience and start having a more consistent approach for JS & interactivity!

edwinthinks avatar Oct 17 '22 22:10 edwinthinks