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

Introduce StimulusReflex Targets #490

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

marcoroth
Copy link
Member

@marcoroth marcoroth commented Apr 14, 2021

Type of PR

Feature

Description

This PR is part of a bigger story. This work is based on #478 and #489.

With this PR we allow to declare DOM nodes as StimulusReflex targets so you are able to access and operate on them with CableRady operations in your reflexes.

Here is an example:

<div data-controller="folder" data-folder-id="42">
  <input type="text" data-reflex-target="folder.input">
  <button data-reflex="click->Folder#search" data-reflex-dataset="combined">Search</button>

  <span id="count" data-reflex-target="folder.count"></span>

  <div id="results" data-reflex-target="folder.results"></div>
</div>
class FolderReflex < StimulusReflex::Reflex
  def search
    controller_element.append(html: '<div id="spinner"></div>')

    input_target.set_attribute(name: 'value', value: '').set_focus
    element.dispatch_event(name: 'search:init').update

    folder = Folder.find_by(id: element.dataset.folder_id)
    results = folder.search(input_target.value)

    count_target.text_content(text: results.count)
    results_target.inner_html(html: render(partial: "folders/results", locals: { results: results }))
 
    element.dispatch_event(name: 'search:complete', detail: { count: results.count })

    cable_ready.remove(selector: '#spinner')

    morph :nothing
  end
end

Why should this be added

Targets are well known to Stimulus developers. This PR applies the same target-concept to reflexes. This, in combination with #489, enables a whole new world on how you write your CableReady operations while writing clean, super understandable and easy-to-look-at code.

Open Points

  • Scoping of Targets
    • currently it just searches for any [data-reflex-target] inside of the controller element
    • Maybe it could make sense to allow targets to be anywhere on the page and not just inside of the controller element.
  • Declaration of Targets (with/without Namespace)
    • Currently it'sfolders.input to declare a target. Should we just allow input?
    • How do we match the namespace to a reflex if we force a namespace?
  • Support for multiple targets with the same name to match the Stimulus behavior?
    • input_target returns the first target
    • input_targets would return an array of all targets so you can loop over them
  • Should we declare targets upfront server side so we can throw errors if a target is not present but accessed?
  • How do we handle the situation if someone wants to use another channel? Or should we limit this API just to the StimulusReflex channel (for now)?

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing
  • This is not a documentation update

@Matt-Yorkley
Copy link
Contributor

Matt-Yorkley commented May 29, 2023

After playing with a working version of this and taking it for a bit of a test drive, I had a few notes and bits of feedback regarding the open questions in the description.

Restricting named targets to the controller element

In the case where there's a custom Stimulus controller on a whole section of the page (maybe a form for example) and it has multiple child elements and calls this.stimulate(), the idea of potentially only including targets that are children of controller_element makes a lot of sense and might be preferable in some cases.

But in the simpler case of a stand-alone element (like a button with data-reflex="click->Example#action") then the element is the controller and the concept of "only include targets that are child elements of the controller" doesn't make sense. In that case it wouldn't be possible to interact with any potential targets, because they would necessarily be outside of controller_element. Know what I mean?

It really feels like it should be at least possible to use named targets that are anywhere on the current page.

Named targets anywhere on the page

If named targets can be anywhere on the page and aren't restricted to being children of controller_element then that creates some other potential issues, like: if the additional data about all named targets on the page is added into every client->server interaction by default would it would probably end up needlessly increasing the wire size in lots of situations where the targets might not even be used/required in a way that doesn't seem preferable.

My thoughts around these questions are coalescing towards the idea that the ability to control whether or not this additional data gets added and what the scope of it should be really wants to be represented by an optional parameter of some kind. Roughly what I'm leaning towards is: by default the additional data about named target elements does not get included. Optionally; data for only the named targets that are children of controller_element can be included by declaring something like include_targets: "controller", or; data about all named targets on the current page can be included with something like include_targets: "page". Either way it feels like it wants to be declared. Maybe on the element? Like...

<span data-reflex-target="whatever"> Target Me! </span>

<button data-reflex="click->Example#action_one"> Not using targets </button>

<button data-reflex="click->Example#action_two" data-include-targets="page"> Targetting! </button>

<form data-controller="stimulating" data-include-targets="controller" data-action="submit->stimulating#submit">
  <input data-reflex-target="launchcodes" type="text" />
  <button data-reflex-target="bigredbutton" class="big red"> Boom! </button>
</from>

Support for multiple targets with the same name

This feels like it would be amazing, but it'll need some changes to how the targets are put together and what the interface for accessing them looks like. Also... there are some interesting questions about calling chainable operations directly on a collection of targets.

I had a quick look and I'm pretty sure that all operations that could feasibly be called on an element have the optional select_all parameter. So... in the case of a collection, if there's an accessor method exposed for something like post_targets which represents a collection of elements with the same target name... then maybe calling post_targets.add_css_class(name: "active") could just automatically set select_all: true when building that operation...? Hmmm... it feels like there's a bit of finesse needed there with regards to the underlying mechanics of it, but I think it can work. It would need the selector argument to be changed to a selector: "[data-reflex-target='<target_name>']" format in that case, but currently it's using xpath when extracting the target data on the client side.

Edit: it works 🎉 👉 Matt-Yorkley@428ac77

A side note on simplicity

An interesting part of this change is that by removing the need to specify the selector argument explicitly (because it gets inserted automatically) it means that certain operations don't require any arguments at all... for example, you could write:

button_target.remove

And that translates to a valid cable_ready operation which does exactly what you'd expect... 👀

There's a lot of scenarios where the reduction in the total volume of code required to achieve things and the overall increase in the clarity and simplicity of that code is absolutely wild.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request proposal ruby Pull requests that update Ruby code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants