data-reflex-dataset and data-reflex-dataset-all #478

leastbad · 2021-03-25T10:33:14Z

Type of PR (feature, enhancement, bug fix, etc.)

Enhancement

Description

@joshleblanc gave me the idea to expand what could be passed to a data-reflex-dataset attribute.

What I've done in this PR is re-imagined the extractElementDataset function as a power tool for grabbing attributes from relative elements.

combined has been deprecated in favour of ancestors. ~~The deprecation itself is commented until #438 is merged.~~

data-reflex-dataset and data-reflex-dataset-all support one or multiple space-separated tokens. eg

data-reflex-dataset="siblings div.success #sleeper parent"

Tokens can be keywords or CSS queries. They are evaluated in the order that they are included.

Currently proposed and supported keywords are:

combined (will emit a deprecation warning)
ancestors
parent
siblings
children
descendants

data-reflex-dataset

Current (3.4.1) behaviour of grabbing and holding the first unique value for a given key is maintained:

<div data-reflex-dataset=".post">
  <div class="post" data-post-id="1"></div>
  <div class="post" data-post-id="2"></div>
  <div class="post" data-post-id="3"></div>
  <div class="post" data-post-id="4"></div>
</div>

The server will receive:

{
 'data-reflex-dataset': '.post',
 'data-post-id': '1'
}

data-reflex-dataset-all

This introduces a 2nd form which enables the scooping of a group of matching elements:

<div data-reflex-dataset-all=".post">
  <div class="post" data-post-id="1"></div>
  <div class="post" data-post-id="2"></div>
  <div class="post" data-post-id="3"></div>
  <div class="post" data-post-id="4"></div>
</div>

The server will receive:

{
 'data-reflex-dataset-alls': ['.post'],
 'data-post-ids': [
   '1',
   '2',
   '3',
   '4'
 ]
}

Since the data-post-id always contains the first occurring value (like it was before) it's fully backwards compatible, because just the data-post-ids is added additionally.

You can of course also mix-and-match the two attributes.

Let's try both!

<div data-reflex-dataset=".post" data-reflex-dataset-all=".post">
  <div class="post" data-post-id="1"></div>
  <div class="post" data-post-id="2"></div>
  <div class="post" data-post-id="3"></div>
  <div class="post" data-post-id="4"></div>
</div>

The server will receive:

{
 'data-reflex-dataset-all': '.post',
 'data-reflex-dataset-alls': ['.post'],
 'data-post-id': '1',
 'data-post-ids': [
   '1',
   '2',
   '3',
   '4'
 ]
}

Smart pluralization

We don't need no postss:

<div data-reflex-dataset-all=".posts">
  <div class="posts" data-post-id="1"></div>
  <div class="posts" data-post-id="2"></div>
  <div class="posts" data-post-id="3"></div>
  <div class="posts" data-post-id="4"></div>
</div>

The server will receive:

{
 'data-reflex-dataset-alls': ['.posts'],
 'data-post-ids': [
   '1',
   '2',
   '3',
   '4'
 ]
}

And thanks to @marcoroth's dark magic, we now have full support for ActiveSupport::Inflector:

<button type="button" data-reflex="click->Example#meta" data-reflex-dataset-all=".post:checked">Meta</button>
<input class="post" type="checkbox" data-wolf="1" checked="true">
<input class="post" type="checkbox" data-wolf="2">
<input class="post" type="checkbox" data-wolf="3" checked="true">
<input class="post" type="checkbox" data-wolf="4">

[1] pry(#<ExampleReflex>)> element.dataset.wolves
=> ["1", "3"]

Wait, did this really just produce a list of all of the checked values? Why, yes it did.

Honestly, it's pretty great

<div class="card-body d-flex justify-content-center" data-age="43">
  <button class="btn btn-dark" type="button" data-reflex="click->Example#meta" data-reflex-dataset-all="siblings parent" data-reflex-dataset="children"><span data-sex="male" class="fas fa-angry me-2"></span>69</button>
  <div class="posts" data-post-id="1" data-name="steve"></div>
  <div class="posts" data-post-id="2" data-name="bill"></div>
  <div class="posts" data-post-id="3" data-name="steve"></div>
  <div class="posts" data-post-id="4" data-name="mike"></div>
</div>

[1] pry(#<ExampleReflex>)> element.dataset
=> #<OpenStruct reflex="click->Example#meta", reflex-dataset-all="siblings parent", reflex-dataset="children", controller="stimulus-reflex", action="click->stimulus-reflex#__perform", sex="male", prefix="fas", icon="angry", fa-i2svg="", reflexs=["click->Example#meta"], reflex-dataset-alls=["siblings parent"], reflex-datasets=["children"], controllers=["stimulus-reflex"], actions=["click->stimulus-reflex#__perform"], post-ids=["1", "2", "3", "4"], names=["steve", "bill", "steve", "mike"], ages=["43"], reflex_dataset_all="siblings parent", reflex_dataset="children", fa_i2svg="", reflex_dataset_alls=["siblings parent"], reflex_datasets=["children"], post_ids=["1", "2", "3", "4"]>

Why should this be added

Power and flexibility.

Thanks to @marcoroth there is ~500LOC worth of tests for the new features of the data-reflex-dataset/data-reflex-dataset-all attributes:

ancestors
parent
siblings
children
descendants

Checklist

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

marcoroth · 2021-03-25T10:47:31Z

Nice, I love the idea to extend data-reflex-dataset!

But I'm a little bit concerned about allowing raw XPath expressions (like/html/body/main/div[2]/div/div/div/div[1]/div[3]/div[5]/input) to the attribute though.

I know that not every XPath is that long, but this feels like an anti-pattern to me and will probably lead to unexpected behavior/hard to maintain code if you change your markup 😅

Maybe just providing simple IDs and classes would work better?

data-reflex-dataset="siblings parent #myinput .checkboxes"

Maybe the classes could be serialized to an array/object. Just thinking out loud.

leastbad · 2021-03-25T11:25:29Z

That's a good point, Marco. I'd be perfectly happy letting default: fall through to document.querySelectorAll instead of doing an XPath lookup. In fact, that's what I should have done in the first place. XPath is ideal for the keywords I provided, but it's not intended for or well suited to being hard-coded. That's what CSS is for. I'll change it!

add jsdom-global to devDependencies and configure mocha to run with it

…13) * add tests for new data-reflex-dataset features * collect and stack overlapping data values in plural key if multiple elements have the `data-post-id` attribute it will put the first occurance in the `data-post-id` key. The first and all the other remaing values of the attribtue will be stacked in a `data-post-ids` array. * handle edge case if the plural key is already used * standardize * extract data attributes stacking to `data-reflex-dataset-array` This is to ensure that you always get an array even if there is just on occurence of an attribute

leastbad · 2021-04-30T04:04:29Z

I'm seeing some interesting results when I interrogate a Reflex element with pry:

<button class="btn btn-dark" type="button" data-reflex="click->Example#meta" data-reflex-dataset=".post"><span class="fas fa-angry me-2"></span>69</button>
<div class="post" data-post-id="1"></div>

Then, in the binding:

[1] pry(#<ExampleReflex>)> element.attributes
=> {"class"=>"btn btn-dark",
 "type"=>"button",
 "data-reflex"=>"click->Example#meta",
 "data-reflex-dataset"=>".post",
 "data-controller"=>"stimulus-reflex",
 "data-action"=>"click->stimulus-reflex#__perform",
 "checked"=>false,
 "selected"=>false,
 "tag_name"=>"BUTTON",
 "value"=>""}
[2] pry(#<ExampleReflex>)> element.dataset
=> #<OpenStruct reflex="click->Example#meta", reflex-dataset=".post", controller="stimulus-reflex", action="click->stimulus-reflex#__perform", post-id="1", reflex_dataset=".post", post_id="1">

Why is the post-id not showing up in element.attributes?
Why do we see a 2nd copy of reflex_dataset=".post", post_id="1" with underscores when I element.dataset?

[3] pry(#<ExampleReflex>)> element.dataset.post_id
=> "1"
[4] pry(#<ExampleReflex>)> element.dataset.reflex_dataset
=> ".post"

The good news is that post_id is accessible. It's so confusing to me that the element.dataset accessor doesn't show reflex_dataset even though it's clearly available, but it does show post_id.

[5] pry(#<ExampleReflex>)> element.dataset["post_id"]
=> "1"
[6] pry(#<ExampleReflex>)> element.dataset["reflex-dataset"]
=> ".post"

It is working, which is good. 👍

marcoroth · 2021-04-30T12:09:46Z

element.attributes just contains the attributes of the element which triggered the reflex. You don't see post_id because data-post-id="1" is on .post and not the element which triggered the reflex.
That's a feature so you can access the data attributes via the dot accessor rather then the array accessor. So you can get the value like this: element.dataset.post_id. This is equivalent to element.dataset["post-id"].

This is happening here: https://github.com/hopsoft/stimulus_reflex/blob/master/lib/stimulus_reflex/element.rb#L10

* split up dataset and datasetArray in payload so we can move the pluralization to the server * move pluralize logic to `StimulusReflex::Element` * move alias_method * extract transform underscore logic in helper method

hopsoft · 2021-05-09T13:57:45Z

I like this a lot and propose that we use list instead of array for the default semantics. Thoughts?

<div data-reflex-dataset-list=".post">
  <div class="post" data-post-id="1"></div>
  <div class="post" data-post-id="2"></div>
  <div class="post" data-post-id="3"></div>
  <div class="post" data-post-id="4"></div>
</div>

The server will receive:

{
 'data-reflex-dataset-lists': ['.post'],
 'data-post-ids': [
   '1',
   '2',
   '3',
   '4'
 ]
}

leastbad · 2021-05-09T14:05:05Z

I'll let you and @marcoroth duke this one out. 🤠

marcoroth · 2021-05-09T14:16:48Z

@hopsoft I guess list would work too, but I have the feeling that array is more common in Ruby-Land.

hopsoft · 2021-05-09T14:29:26Z

I was thinking in terms of dataset == object/hash and list == array with an eye toward the mindset people are likely to have when writing the declarative markup, but I definitely want to err on whatever is most intuitive for the user.

Would love to hear other opinions on this.

leastbad added 2 commits March 24, 2021 22:36

no more passing schema into functions

f39d0cb

extractElementDataset v2

e2069ea

default case falls through to css instead of xpath

f9492ae

leastbad changed the title ~~Dataset~~ data-reflex-dataset supports multiple relative keywords Mar 25, 2021

leastbad added this to the 3.4.2 milestone Mar 25, 2021

leastbad added deprecated deprecates a feature javascript Pull requests that update Javascript code proposal labels Mar 25, 2021

This was referenced Apr 14, 2021

Allow to execute CableReady operations on StimulusReflex::Element #489

Open

Introduce StimulusReflex Targets #490

Draft

leastbad and others added 7 commits April 28, 2021 04:12

Merge branch 'master' into dataset

b067c80

uncommented the deprecation warning

f66d284

add jsdom-global to devDependencies and configure mocha to run with it

5e04abd

Merge pull request #12 from marcoroth/leastbad/dataset

19ef7a1

add jsdom-global to devDependencies and configure mocha to run with it

import schema to allow testing attributes

a923795

extractElementDataset only takes one argument, now

3cda547

leastbad changed the title ~~data-reflex-dataset supports multiple relative keywords~~ data-reflex-dataset and data-reflex-dataset-array Apr 30, 2021

move pluralize logic to StimulusReflex::Element (#14)

ef4325f

* split up dataset and datasetArray in payload so we can move the pluralization to the server * move pluralize logic to `StimulusReflex::Element` * move alias_method * extract transform underscore logic in helper method

array -> all

73bcb50

hopsoft approved these changes May 10, 2021

View reviewed changes

leastbad changed the title ~~data-reflex-dataset and data-reflex-dataset-array~~ data-reflex-dataset and data-reflex-dataset-all May 10, 2021

leastbad merged commit 630b1b8 into stimulusreflex:master May 10, 2021

leastbad mentioned this pull request Jun 25, 2021

Calling Reflex with data-action and data-reflex-dataset='combined' send combined dataset with event.currentTarget only #525

Closed

leastbad deleted the dataset branch August 16, 2021 03:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

data-reflex-dataset and data-reflex-dataset-all #478

data-reflex-dataset and data-reflex-dataset-all #478

leastbad commented Mar 25, 2021 •

edited

Loading

marcoroth commented Mar 25, 2021

leastbad commented Mar 25, 2021

leastbad commented Apr 30, 2021 •

edited

Loading

marcoroth commented Apr 30, 2021

hopsoft commented May 9, 2021

leastbad commented May 9, 2021

marcoroth commented May 9, 2021

hopsoft commented May 9, 2021

data-reflex-dataset and data-reflex-dataset-all #478

data-reflex-dataset and data-reflex-dataset-all #478

Conversation

leastbad commented Mar 25, 2021 • edited Loading

Type of PR (feature, enhancement, bug fix, etc.)

Description

data-reflex-dataset

data-reflex-dataset-all

Let's try both!

Smart pluralization

Honestly, it's pretty great

Why should this be added

Checklist

marcoroth commented Mar 25, 2021

leastbad commented Mar 25, 2021

leastbad commented Apr 30, 2021 • edited Loading

marcoroth commented Apr 30, 2021

hopsoft commented May 9, 2021

leastbad commented May 9, 2021

marcoroth commented May 9, 2021

hopsoft commented May 9, 2021

leastbad commented Mar 25, 2021 •

edited

Loading

leastbad commented Apr 30, 2021 •

edited

Loading