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

Proposal: data-reflex-disable data-reflex-disable-with #229

Closed

Conversation

jasoncharnes
Copy link
Contributor

@jasoncharnes jasoncharnes commented Jun 8, 2020

Feature

Description

When I was pairing with Nate on the params stuff, someone mentioned how it would be cool to have a data-disable-with like Rails has in rails-ujs. (I think that's where it comes from? 🤔)

This PR introduces:

  1. data-reflex-disable to disable an element when a reflex fires, and re-enables the element after the reflex completes
  2. data-reflex-disable-with to disable an element and update the innerHTML when a reflex fires, and re-enables and reset innerHTML when the reflex completes

data-reflex-disable

<!-- The view -->
<button data-reflex="PartyReflex#dance" data-reflex-disable>Dance!</button>

<!-- The view during the reflex process -->
<button data-reflex="PartyReflex#dance" data-reflex-disable disabled>Dance!</button>

<!-- The view once the reflex process completes -->
<button data-reflex="PartyReflex#dance" data-reflex-disable>Dance!</button>

data-reflex-disable-with

<!-- The view -->
<button data-reflex="PartyReflex#dance" data-reflex-disable-with="Dancing! 🕺">Dance!</button>

<!-- The view during the reflex process -->
<button data-reflex="PartyReflex#dance" data-reflex-disable-with="Dancing! 🕺" data-reflex-enable-with="Dance!" disabled>Dancing! 🕺</button>

<!-- The view once the reflex process completes -->
<button data-reflex="PartyReflex#dance" data-reflex-disable-with="Dancing! 🕺">Dance!</button>

Why should this be added

This would save callbacks solely for disabling/enabling elements (which we do a lot, currently, at Podia) and is in line with Rails functionality.

Why didn't you just name this data-disable-with?

I don't want to interfere with Rails and I like that it's reflex-specific. If the consensus is that it should follow that name, I'm more than glad to update it. 🤗

Help!

This is a naive implementation, and currently only is tested on buttons.

  1. What other elements should this feature support?
  2. Is the code in the right place?
  3. can the implementation be improved?

Checklist

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

@leastbad
Copy link
Contributor

leastbad commented Jun 8, 2020

This is cool. No surprise there. 😉

QQ: did you develop this with Nate? It’s not clear from the intro.

So: early on I went through the LiveView client source line-by-line, looking for wisdom and potential future gotchas. [It’s since been re-written, and in a way that is far less 🍿 which is super sad.] I was making a list of things they do which we could do, too - and disable-with was on it. So was append/prepend support.

I quickly made a PR to add append/prepend, and Nate calmly pointed out that because of the flexible architecture of SR, we technically already had that feature via CableReady.

I’m now returning the favor by pointing out that unlike LiveView we have Stimulus and SR custom callback events. Couldn’t you implement a Stimulus ApplicationController that you could inherit from that would add this functionality without having to add anything to StimulusReflex?

@jasoncharnes
Copy link
Contributor Author

QQ: did you develop this with Nate? It’s not clear from the intro.

I didn't. Sorry that wasn't clear! We paired on params and this was something that was brought up, but I also can't remember if he brought it up or I did. 😆 I just remembered that when working through our codebase.

Couldn’t you implement a Stimulus ApplicationController that you could inherit from that would add this functionality without having to add anything to StimulusReflex?

I haven't thought of that, but yeah- I could totally do that. 👍 I don't have a solid argument for/against this making it into SR, but while we're here I'll see if this is something others would want in the library.

If this isn't functionality we wan't to bring into SR, I'll absolutely be doing rolling with your suggestion locally!

@leastbad
Copy link
Contributor

leastbad commented Jun 8, 2020

Oh, so... name-dropping then... 😄 I kid, I kid.

We are somewhat protective of adding things to the core just because they are cool; moreso if it involves embiggening the footprint of the custom attribute schema.

However, the main reason I'm questioning this is because I see it as an opportunity to nudge you gently towards a third path we're getting pretty excited about. Essentially, we've realized that there's only a relatively small group of people actively evangelizing Stimulus and well, we're all on a first-name basis.

What we're building up to is launching a CPAN-style resource of composable Stimulus controllers sometime this summer. When I saw your PR this morning, I immediately thought of @adrienpoly's awesome stimulus-use project, specifically his ApplicationController.

The exciting third path option here would be to see this idea get developed into a Stimulus controller that you publish on npm as stimulus-reflex-disable or something similar. And then we promote it as a blessed technique. 0 LOC added!

@jasoncharnes
Copy link
Contributor Author

@leastbad I've looked at stimulus-use but I haven't used it, yet. I think a stimulus-reflex-disable package could pair nicely with a tool like that!

I'm going to close this for now, and if there's interest I can always re-open it. :)

Thanks for the tips!

@leastbad
Copy link
Contributor

How are you making out on the npm package version of this? I can’t wait to try it out!

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.

2 participants