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

Support refs in tags #1185

Closed
steelbrain opened this Issue Sep 3, 2015 · 21 comments

Comments

Projects
None yet
10 participants
@steelbrain
Contributor

steelbrain commented Sep 3, 2015

React supports them and they are super-cool. They make complex DOM manipulation less painful.

<div ...>
  <div ...>
    <div ...>
      <div ...>
        <span ref="something">Wow</span>
      </div>
      ..
    </div>
    ...
  </div>
  ...
</div>
console.log(this.refs.something) // <-- will point to that span
@cognitom

This comment has been minimized.

Show comment
Hide comment
@cognitom

cognitom Sep 3, 2015

Member

Hi. I like that idea. We have some discussion on #715 (comment) .

Member

cognitom commented Sep 3, 2015

Hi. I like that idea. We have some discussion on #715 (comment) .

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Sep 3, 2015

Member

I also like this idea, I will try include this feature in riot 2.3.0, but there is still a lot of work to do

Member

GianlucaGuarini commented Sep 3, 2015

I also like this idea, I will try include this feature in riot 2.3.0, but there is still a lot of work to do

@nippur72

This comment has been minimized.

Show comment
Hide comment
@nippur72

nippur72 Sep 3, 2015

Contributor

Regarding ref= I like it too, but there will be still support for id= or name= ?

Contributor

nippur72 commented Sep 3, 2015

Regarding ref= I like it too, but there will be still support for id= or name= ?

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Sep 3, 2015

Member

I thought about it and I would like plan this switch for riot 2.4.0 that will be a bit far from the next release. Riot 2.3.0 will change a lot the codebase and I would like to avoid introducing big api changes. Let's come back on this once riot 2.3.* will become enough stable

Member

GianlucaGuarini commented Sep 3, 2015

I thought about it and I would like plan this switch for riot 2.4.0 that will be a bit far from the next release. Riot 2.3.0 will change a lot the codebase and I would like to avoid introducing big api changes. Let's come back on this once riot 2.3.* will become enough stable

@cognitom

This comment has been minimized.

Show comment
Hide comment
@cognitom

cognitom Sep 3, 2015

Member

👍 v2.4.0

Member

cognitom commented Sep 3, 2015

👍 v2.4.0

@GianlucaGuarini GianlucaGuarini added this to the 2.4.0 milestone Nov 25, 2015

@RanzQ

This comment has been minimized.

Show comment
Hide comment
@RanzQ

RanzQ Feb 17, 2016

Contributor

Is this implemented in 3.0.0?

Contributor

RanzQ commented Feb 17, 2016

Is this implemented in 3.0.0?

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Feb 17, 2016

Member

I thought about and probably I will not implement it in the next major release to avoid breaking changes

Member

GianlucaGuarini commented Feb 17, 2016

I thought about and probably I will not implement it in the next major release to avoid breaking changes

@GianlucaGuarini GianlucaGuarini removed this from the 2.4.0 milestone Mar 3, 2016

@cognitom cognitom self-assigned this Jun 24, 2016

@cognitom cognitom added this to the 3.0.0 milestone Jun 24, 2016

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Jun 25, 2016

Member

things are changed and this feature will land in riot 3.0.0

Member

GianlucaGuarini commented Jun 25, 2016

things are changed and this feature will land in riot 3.0.0

@GianlucaGuarini GianlucaGuarini referenced this issue Jun 25, 2016

Closed

Riot 3.0.0 roadmap #1694

14 of 16 tasks complete
@avimar

This comment has been minimized.

Show comment
Hide comment
@avimar

avimar Jul 12, 2016

Since id= and name= already seem to be accessible via this.SomeID, what are we doing here?

GianlucaGuarini #1694 (comment)

Another big update will be the use of refs instead of name or id to better scope the DOM elements queries under the same path this.refs.myDomNode

Are we replacing id and name access with required ref instead? It was rather nifty that for form elements I can simply use name and then it was accessible via riot. And for other elements, if I wanted to name them, just add an ID.
I initially read GianlucaGuarini's comment as saying that IDs/names were moving to be namespaced under ref -- which might help avoid conflicting names of variables, but I see that's not what he meant.

avimar commented Jul 12, 2016

Since id= and name= already seem to be accessible via this.SomeID, what are we doing here?

GianlucaGuarini #1694 (comment)

Another big update will be the use of refs instead of name or id to better scope the DOM elements queries under the same path this.refs.myDomNode

Are we replacing id and name access with required ref instead? It was rather nifty that for form elements I can simply use name and then it was accessible via riot. And for other elements, if I wanted to name them, just add an ID.
I initially read GianlucaGuarini's comment as saying that IDs/names were moving to be namespaced under ref -- which might help avoid conflicting names of variables, but I see that's not what he meant.

@MartinMuzatko

This comment has been minimized.

Show comment
Hide comment
@MartinMuzatko

MartinMuzatko Jul 12, 2016

Member

There are pros and cons for that. Maybe we can stick to name and id but scope them to this.elements but this is additional brainload IMHO.

Pros:

  • I don't mix it up with events anymore
<app>
    <div name="filter" onkeyup={filter}>
    </div>
    <script>
        filter(e) {
            this.filter.classList.add('expand')
        }
    </script>
</app>
  • Everything is scoped (tags, opts)
  • Less interference with loops (each={shortcut in shortcuts} name="shortcut"

Cons:

  • More brainload, in components usually you can distinguish between events and elements with naming conventions
  • Should we namespace everything now? what is left in the tag scope are events (this.filter())

I think this heavily depends on personal taste, what should belong to the local tag and what should be namespaced (like this.el)

Also, there is already a confusion between elements and tags. Since riot calls component tags but we can also select non-riot tags within our components.

Regarding the name: I personally prefer @tipiirai s suggestion: #715 (comment) with this.dom

As for this.refs, I'd associate it more with references to plain JS objects, rather than DOM objects.

Member

MartinMuzatko commented Jul 12, 2016

There are pros and cons for that. Maybe we can stick to name and id but scope them to this.elements but this is additional brainload IMHO.

Pros:

  • I don't mix it up with events anymore
<app>
    <div name="filter" onkeyup={filter}>
    </div>
    <script>
        filter(e) {
            this.filter.classList.add('expand')
        }
    </script>
</app>
  • Everything is scoped (tags, opts)
  • Less interference with loops (each={shortcut in shortcuts} name="shortcut"

Cons:

  • More brainload, in components usually you can distinguish between events and elements with naming conventions
  • Should we namespace everything now? what is left in the tag scope are events (this.filter())

I think this heavily depends on personal taste, what should belong to the local tag and what should be namespaced (like this.el)

Also, there is already a confusion between elements and tags. Since riot calls component tags but we can also select non-riot tags within our components.

Regarding the name: I personally prefer @tipiirai s suggestion: #715 (comment) with this.dom

As for this.refs, I'd associate it more with references to plain JS objects, rather than DOM objects.

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Jul 26, 2016

Member

@cognitom Any progress on this feature? I would like to test if removing the named id/names DOM queries we will get better performances

Member

GianlucaGuarini commented Jul 26, 2016

@cognitom Any progress on this feature? I would like to test if removing the named id/names DOM queries we will get better performances

@MartinMuzatko

This comment has been minimized.

Show comment
Hide comment
@MartinMuzatko

MartinMuzatko Jul 28, 2016

Member

There is still something I'm confused about:

React supports them and they are super-cool. They make complex DOM manipulation less painful.

How is this.refs.element less painful than this.element ??
How does the "usual" DOM manipulation compare to "complex" DOM manipulation with this.refs? I feel like I'm missing something here.

This has nothing really to do with support or cool, but to namespace the elements, or am I missing any other features that would get introduced with this.refs?

Please help resolve the confusion @GianlucaGuarini.

Member

MartinMuzatko commented Jul 28, 2016

There is still something I'm confused about:

React supports them and they are super-cool. They make complex DOM manipulation less painful.

How is this.refs.element less painful than this.element ??
How does the "usual" DOM manipulation compare to "complex" DOM manipulation with this.refs? I feel like I'm missing something here.

This has nothing really to do with support or cool, but to namespace the elements, or am I missing any other features that would get introduced with this.refs?

Please help resolve the confusion @GianlucaGuarini.

@nippur72

This comment has been minimized.

Show comment
Hide comment
@nippur72

nippur72 Jul 28, 2016

Contributor

I'm also confused, Riot already has DOM elements referencing by id and name, so why ref ?

BTW, React 15 has dropped support for ref with string argument. It does support only ref=function() and you have to manage the reference by yourself. I guess they did that because they are promoting a stronger type checking on components.

Contributor

nippur72 commented Jul 28, 2016

I'm also confused, Riot already has DOM elements referencing by id and name, so why ref ?

BTW, React 15 has dropped support for ref with string argument. It does support only ref=function() and you have to manage the reference by yourself. I guess they did that because they are promoting a stronger type checking on components.

@cognitom

This comment has been minimized.

Show comment
Hide comment
@cognitom

cognitom Jul 28, 2016

Member

@GianlucaGuarini sorry for my late reply. I think almost done. I'm fixing the tests, right now.

But, what I did was so simple. Just two lines, named.js and parse.js:

arrayishAdd(customParent, value, tagOrDom) // old
arrayishAdd(customParent.refs, value, tagOrDom) // new
// old
if (name === 'name' || name === 'id') {
  expr = new NamedExpr(dom, name, attr.value, tag) }
// new
if (name === 'name' || name === 'id' || name === 'ref') {
  expr = new NamedExpr(dom, name, attr.value, tag)
}

Anyway I'll send the PR ASAP. Let me know if there's anything missing.

Member

cognitom commented Jul 28, 2016

@GianlucaGuarini sorry for my late reply. I think almost done. I'm fixing the tests, right now.

But, what I did was so simple. Just two lines, named.js and parse.js:

arrayishAdd(customParent, value, tagOrDom) // old
arrayishAdd(customParent.refs, value, tagOrDom) // new
// old
if (name === 'name' || name === 'id') {
  expr = new NamedExpr(dom, name, attr.value, tag) }
// new
if (name === 'name' || name === 'id' || name === 'ref') {
  expr = new NamedExpr(dom, name, attr.value, tag)
}

Anyway I'll send the PR ASAP. Let me know if there's anything missing.

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Jul 28, 2016

Member

@cognitom that's wrong. The ref should be cached in this.ref avoiding conflicts with other components properties. The idea is to remove the crap coming from the DOM that automatically gets set as component property generating stupid errors.
The name and id attributes should be removed as well. Let me know if it's too much work

Member

GianlucaGuarini commented Jul 28, 2016

@cognitom that's wrong. The ref should be cached in this.ref avoiding conflicts with other components properties. The idea is to remove the crap coming from the DOM that automatically gets set as component property generating stupid errors.
The name and id attributes should be removed as well. Let me know if it's too much work

@cognitom

This comment has been minimized.

Show comment
Hide comment
@cognitom

cognitom Jul 28, 2016

Member

@GianlucaGuarini thanks for clarification.

The ref should be cached in this.ref

customParent.refs is this.refs. Do you mean this.ref instead of this.refs?

The name and id attributes should be removed as well.

OK, I'll remove them, too.
(If I haven't done in tonight, I'll let you know)

Member

cognitom commented Jul 28, 2016

@GianlucaGuarini thanks for clarification.

The ref should be cached in this.ref

customParent.refs is this.refs. Do you mean this.ref instead of this.refs?

The name and id attributes should be removed as well.

OK, I'll remove them, too.
(If I haven't done in tonight, I'll let you know)

@rsbondi

This comment has been minimized.

Show comment
Hide comment
@rsbondi

rsbondi Jul 28, 2016

Member

@MartinMuzatko this does 2 things, namespacing as you mentioned but also, there are other reasons to use name and id, so this would be more explicit, and not bog down in these cases.

I have seen many breakages when using a class as a selector with jquery, where one might expect class to be for layout, and someone works on the layout and changes the class and the code breaks. This would remove this type of overlapping intention.

Member

rsbondi commented Jul 28, 2016

@MartinMuzatko this does 2 things, namespacing as you mentioned but also, there are other reasons to use name and id, so this would be more explicit, and not bog down in these cases.

I have seen many breakages when using a class as a selector with jquery, where one might expect class to be for layout, and someone works on the layout and changes the class and the code breaks. This would remove this type of overlapping intention.

@rongxike

This comment has been minimized.

Show comment
Hide comment
@rongxike

rongxike Nov 28, 2016

I cannot upgrade to 3.0.1 because of this refs, all of my team's projects are using this.inputID (more than 1000 lines of code). Now, it's really painful to change everything to ref="inputID" and refs.inputID

Could you please put this.inputID back to peace?

rongxike commented Nov 28, 2016

I cannot upgrade to 3.0.1 because of this refs, all of my team's projects are using this.inputID (more than 1000 lines of code). Now, it's really painful to change everything to ref="inputID" and refs.inputID

Could you please put this.inputID back to peace?

@MartinMuzatko

This comment has been minimized.

Show comment
Hide comment
@MartinMuzatko

MartinMuzatko Nov 28, 2016

Member

@rongxike which Riot 3.x features would you need that stops you to stay with 2.6.x? 3.x introduces breaking changes - and they have all right to do so.

If you want to migrate to 3.x, you could use regular expressions to get your IDs and Names and replace them with refs.
Migrating to a new major version always means extra work. (Imagine telling Angular2 to revert to Angular1 style)
Please ask in our gitter channel if you need further support.

Member

MartinMuzatko commented Nov 28, 2016

@rongxike which Riot 3.x features would you need that stops you to stay with 2.6.x? 3.x introduces breaking changes - and they have all right to do so.

If you want to migrate to 3.x, you could use regular expressions to get your IDs and Names and replace them with refs.
Migrating to a new major version always means extra work. (Imagine telling Angular2 to revert to Angular1 style)
Please ask in our gitter channel if you need further support.

@sourcegr

This comment has been minimized.

Show comment
Hide comment
@sourcegr

sourcegr Nov 28, 2016

sourcegr commented Nov 28, 2016

@rongxike

This comment has been minimized.

Show comment
Hide comment
@rongxike

rongxike Nov 29, 2016

Thank you for your replies, i will try my best to update my codes.

rongxike commented Nov 29, 2016

Thank you for your replies, i will try my best to update my codes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment