refactor: namespace custom js events with bulk-actions

danieldpence commented 2 years ago

The bulk_actions_controller dispatches the following custom javascript events: "toggle" and "update-ids". This could cause an unintentional naming collision if a developer is listening for a toggle event that is emitted elsewhere on the page.

This PR namespaces these events as bulk-actions:toggle and bulk-actions:update-ids, which should hopefully avoid event name collisions.

pascallaliberte commented 2 years ago

@danieldpence Good catch, you're right this is the way to go.

I also recommend renaming bulk-actions:toggle with bulk-actions:toggle-select-all-label for clarity.

jagthedrummer commented 1 year ago

@danieldpence @pascallaliberte What's the status on this one? Do we want to do the suggestion of renaming bulk-actions:toggle with bulk-actions:toggle-select-all-label or should we just merge it as is?

danieldpence commented 1 year ago

@jagthedrummer wow, this one fell off my radar for sure. I went ahead and made the update as @pascallaliberte suggested. Should be good to go now, I think?

pascallaliberte commented 1 year ago

👍 this one is good to go

jagthedrummer commented 1 year ago

@pascallaliberte @danieldpence Do either of you have enough experience with this repo to look into why tests are failing here and on main? I'd love to get tests passing again so that we can have more confidence in merging.

jagthedrummer commented 3 months ago

@danieldpence and @pascallaliberte, I was trying to get this ready to merge but there is a failing test that seems legit.

To confirm I did this:

rails g super_scaffold Project Team name:text_field --navbar="ti-world"
rails db:migrate
rails g super_scaffold:action_models:targets_many Archive Project Team
rails db:migrate

And then when I go to the app and create a few Projects, nothing happens when I select some of them:

After much confusion I finally realized that we have two copies of a couple of important javascript files across the BT repo-system.

app/javascript/controllers/bulk_action_form_controller.js and app/javascript/controllers/bulk_actions_controller.js can be found in the core repo and in this repo.

https://github.com/bullet-train-co/bullet_train-core/tree/main/bullet_train/app/javascript/controllers

https://github.com/bullet-train-pro/bullet_train-action_models/tree/main/app/javascript/controllers

It looks like the one from core is what's actually used by apps. We don't seem to be publishing an npm package for ActionModels so I don't think there's any way for the JS in this repo to be useful for anyone as it stands right now.

Does what I've said above seem right to you all? Am I missing something?

jagthedrummer commented 3 months ago

OK, to test this out I made a ddp/namespace-js-events branch in core and then made this same set of changes to the JS files that are duplicated there. After doing that a different test in this PR started failing.

I found a couple of more spots in views that needed to be namespaced and I fixed those: https://github.com/bullet-train-pro/bullet_train-action_models/pull/1/commits/323923f55ac04a037182b2d9694e894cc6634592

And now the tests are passing.

I think we should probably remove the duplicate JS files from this repo to prevent any future confusion: https://github.com/bullet-train-pro/bullet_train-action_models/pull/1/commits/0792c70a58d37d3330dc42771188542a6ca91687

I also did some quick grepping through core to see if there are any views in that repo that need the same type of namspacing applied, but I didn't find anything.

So I think we should be good to merge this PR and the joint PR in core: https://github.com/bullet-train-co/bullet_train-core/pull/855

Let me know if I'm missing something.

bullet-train-pro / bullet_train-action_models

refactor: namespace custom js events with bulk-actions #1