Skip to content

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

Merged
leastbad merged 12 commits into
stimulusreflex:masterfrom
leastbad:dataset
May 10, 2021
Merged

data-reflex-dataset and data-reflex-dataset-all#478
leastbad merged 12 commits into
stimulusreflex:masterfrom
leastbad:dataset

Conversation

@leastbad

@leastbad leastbad commented Mar 25, 2021

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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!

@leastbad leastbad changed the title Dataset data-reflex-dataset supports multiple relative keywords Mar 25, 2021
@leastbad leastbad added this to the 3.4.2 milestone Mar 25, 2021
@leastbad leastbad added deprecated deprecates a feature javascript Pull requests that update Javascript code proposal labels Mar 25, 2021
leastbad and others added 7 commits April 28, 2021 04:12
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 leastbad changed the title data-reflex-dataset supports multiple relative keywords data-reflex-dataset and data-reflex-dataset-array Apr 30, 2021
@leastbad

leastbad commented Apr 30, 2021

Copy link
Copy Markdown
Contributor Author

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">
  1. Why is the post-id not showing up in element.attributes?
  2. 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

Copy link
Copy Markdown
Member
  1. 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.
  2. 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

hopsoft commented May 9, 2021

Copy link
Copy Markdown
Contributor

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

leastbad commented May 9, 2021

Copy link
Copy Markdown
Contributor Author

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

@marcoroth

Copy link
Copy Markdown
Member

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

@hopsoft

hopsoft commented May 9, 2021

Copy link
Copy Markdown
Contributor

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 leastbad changed the title data-reflex-dataset and data-reflex-dataset-array data-reflex-dataset and data-reflex-dataset-all May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deprecated deprecates a feature javascript Pull requests that update Javascript code proposal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants