webtides / element-js

Simple and lightweight base classes for web components with a beautiful API
MIT License
28 stars 3 forks source link

feat($$refs): adds $$refs map to base Element #39 #41

Closed quarkus closed 2 years ago

quarkus commented 2 years ago

This is just supposed to be a draft.

Actually i would be happier if we would ditch $ref and $refs in favor of $ and $$ but that would be breaking.

any thoughts ?

eddyloewen commented 2 years ago

To be honest I don't like all the $ signs in my code at all. It reminds me of either jQuery or PHP code...

I like the idea though! Just for the record I would like to add a few thoughts.

How about we call them $refs and $refsAll ? That would kind of mimic the querySelector and querySelectorAll functionality.

Also I'm a bit wary about the magic that is being added here when querying for multiple refs. It's probably fine. But what if we made it more explicit by using square brackets? Just like for multiple values (radio) in forms.

<p ref=unique></p>
<p ref="[notsounique]">notunique1</p>
<p ref="[notsounique]">notunique2</p>
quarkus commented 2 years ago

To be honest I don't like all the $ signs in my code at all. It reminds me of either jQuery or PHP code...

I like the idea though! Just for the record I would like to add a few thoughts.

How about we call them $refs and $refsAll ? That would kind of mimic the querySelector and querySelectorAll functionality.

Also I'm a bit wary about the magic that is being added here when querying for multiple refs. It's probably fine. But what if we made it more explicit by using square brackets? Just like for multiple values (radio) in forms.

<p ref=unique></p>
<p ref="[notsounique]">notunique1</p>
<p ref="[notsounique]">notunique2</p>

Hmm i don't think i like the idea of having another "custom" name ($refsAll). And you'll keep one $ anyways :) !

I was actually naming it with the chrome console selector in mind which i find quite declarative: https://developer.chrome.com/docs/devtools/console/utilities/#querySelectorAll-function So I'd like to put this up for discussion (@lukas-schardt , @KinG-DE ).

$$refs or $refsAll ?

As for the Brackets ... yeah .. but why ? The code behind the selection would almost be the same just filtered. I'll go with #TeamMagic :D !

KinG-DE commented 2 years ago

I think, both options are kind of weird and clunky. On one hand you have $$refs that let me cringe each time I write that. And on the other hand you have $refsAll, which suggest with refs that you are already accessing all refs.

Nevertheless, I would go with the $$refs. I think it feels better from a writing standpoint and with the browser behaviour in mind.

lukas-schardt commented 2 years ago

And keeping it all inside $refs is not an option? This way we don't have to introduce another variable and we can easily ditch the second querySelectorAll inside the registerRefs function. And the first ref found won't be both inside the $refs map and inside the $refsAllMap.

Then if we have multiple refs, it will be be an array of references and if we have only one ref it will be the reference

image

        refsArray.forEach((refNode) => {
            const refKey = refNode.getAttribute('ref');
            if (refsMap.hasOwnProperty(refKey)) {
                refsMap[refKey] = Array.isArray(refsMap[refKey])
                    ? [...refsMap[refKey], refNode]
                    : [refsMap[refKey], refNode];
            } else {
                refsMap[refKey] = refNode;
            }
        });

The developer using the $refs functionality should now if multiple ref-Attributes with the same name are set.

quarkus commented 2 years ago

@lukas-schardt I did not do that cause this is be potentially breaking as well.

Imagine a developer declared a ref twice in a component in the past. Would be an error, but it will also break if we just change the reference to the element to a collection with this release.

As it all comes down to naming and opinion we should probably come up with sth. in a call. Beer might help :) !

cheers

quarkus commented 2 years ago

Conclusion:

We'll stick with the current $ref name. References to a NodeList will be explicitly defined with a dangling "[]" within the refs value.

<p ref=unique></p>
<p ref="notsounique[]">notunique1</p>
<p ref="notsounique[]">notunique2</p>

Codewise we'll check if the refs value contains the dangling "[]", create an Array, push the current Element as well as the following occurrences with the same value.