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

Tests for CableReady::Identifiable and CableReady::OperationBuilder #4

Merged
merged 3 commits into from
Apr 6, 2021

Conversation

marcoroth
Copy link

@marcoroth marcoroth commented Apr 2, 2021

Here is a proposal to further improve the awesome PR stimulusreflex#107.

This PR adds:

  • a new CableReady::OperationBuilder#selector method

    This method can be useful to explicit define which selector should be used. Previously the previous_selector was just set implicitly if you called another operation before. With #selector you can explicitly control the selector.

  • tests for the above mentioned CableReady::OperationBuilder#selector

  • more tests for CableReady::OperationBuilder

  • a small refactoring for CableReady::Identifiable

  • a ton of tests for CableReady::Identifiable

@leastbad
Copy link
Owner

leastbad commented Apr 2, 2021

There's lots to love here, but let me explain my reasoning behind previous_selector.

In today's CR, selector is a mandatory option in about 2/3rds of the operations, and it's been my experience that often a chain of operations ends up selecting the same element multiple times. By giving the chain a bit of memory, you can opt-in to the behaviour by writing less code. The simplest way to not by implicit is to just continue to do what you already do, today. I love this because it means there's no additional syntax to learn and it's actually discoverable.

There's also the relatively undiscussed stimulusreflex#110 which I recognize isn't as sexy but it comes with a very interesting capability, which is the opportunity to stagger individual operations. This provides developers with a chance to implement chains of functionality that fire off in a sequence. Being able to send a time-staggered set of operations to the same element with very little code is exciting - it's the sort of thing I use to build interfaces that feel alive. (I know this doesn't directly contradict a selector method but I'm trying to paint a picture of a really clean and sexy API.)

The main issue I have with this you probably saw coming because it's the same issue I had with stimulusreflex#91, which is that it introduces something that looks like an operation but isn't. I'm actually really concerned that this will be the straw that breaks the camel's back when it comes to people wrapping their heads around the library. So, while I can be honest and say that I don't love the proposed change, the API syntax is a big enough deal that I would be trying to find another way to do this even if I did love it.


Code feedback!

Only after typing all of the above, I realize/think that you're proposing the selector method in addition to the implicit selector, not instead of, right? I'm still 👎 for all of the reasons above, but I want to fully understand.

Why do you need to require active_record in operation_builder?

Why did you remove the prefix from the dom_id call? Prefixes are important, I think!

Actually, I just had that 🤯 feeling because I think you didn't remove prefixes, you just saw a higher-level abstraction for all three cases that I didn't previously perceive. So: can you verify that you didn't change the functionality, just improved the implementation? If so, I can get used to that!

I think that's everything. Thanks again, Marco!

@marcoroth marcoroth changed the title introduce CableReady::OperationBuilder#selector + Tests Tests for CableReady::Identifiable? and CableReady::OperationBuilder` Apr 6, 2021
@marcoroth marcoroth changed the title Tests for CableReady::Identifiable? and CableReady::OperationBuilder` Tests for CableReady::Identifiable and CableReady::OperationBuilder Apr 6, 2021
@marcoroth
Copy link
Author

marcoroth commented Apr 6, 2021

As discussed on Discord:

  • I removed the #selector method
  • I removed the hash and delimiter params fromdom_id

@leastbad leastbad merged commit 450ff61 into leastbad:memory Apr 6, 2021
@marcoroth marcoroth deleted the memory branch April 6, 2021 23:20
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