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

yield: show={expr} has different scope than if={expr} #2158

Closed
juodumas opened this Issue Dec 14, 2016 · 6 comments

Comments

Projects
None yet
4 participants
@juodumas
Contributor

juodumas commented Dec 14, 2016

  1. Describe your issue:
    Expression in show={expression} has different scope than if={expression}, onclick={expression} etc. inside <yield>. Looks like the problems was introduced in riot-3.0.3 while fixing riot/riot#2125.

  2. Can you reproduce the issue?
    http://plnkr.co/edit/Z6c36rPdnhplBVNN6JQ0?p=preview

  3. On which browser/OS does the issue appear?
    Chromium 54

  4. Which version of Riot does it affect?
    riot@v3.0.4

  5. How would you tag this issue?

  • Bug
@TimWillis

This comment has been minimized.

Show comment
Hide comment
@TimWillis

TimWillis Dec 16, 2016

This one is getting me as well on Riot 3.0.4 - just switched to riot 3.0 and started updating everything, then jumped to 3.0.4 to hopefully help fix the bugs I got from going to 2 to 3 and now more bugs with this issue...

TimWillis commented Dec 16, 2016

This one is getting me as well on Riot 3.0.4 - just switched to riot 3.0 and started updating everything, then jumped to 3.0.4 to hopefully help fix the bugs I got from going to 2 to 3 and now more bugs with this issue...

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Dec 16, 2016

Member

@riot/collaborators the current yield behavior should be fixed I would like to put it on the top of our priority list together with performance improvements. Thanks @juodumas for reporting this issue

Member

GianlucaGuarini commented Dec 16, 2016

@riot/collaborators the current yield behavior should be fixed I would like to put it on the top of our priority list together with performance improvements. Thanks @juodumas for reporting this issue

@TimWillis

This comment has been minimized.

Show comment
Hide comment
@TimWillis

TimWillis Dec 23, 2016

On line 1135 in riotjs I did this:

// the value for the toggle must consider also the parent tag
//   value = isToggle ? tmpl(expr.expr, extend({}, this, this.parent)) : tmpl(expr.expr, this),
  value = tmpl(expr.expr, this),

I believe someone put this in to fix an issue, but I am not having that issue that I can find, and this fixed the issue here. Hope this helps solve the problem for the next release.

TimWillis commented Dec 23, 2016

On line 1135 in riotjs I did this:

// the value for the toggle must consider also the parent tag
//   value = isToggle ? tmpl(expr.expr, extend({}, this, this.parent)) : tmpl(expr.expr, this),
  value = tmpl(expr.expr, this),

I believe someone put this in to fix an issue, but I am not having that issue that I can find, and this fixed the issue here. Hope this helps solve the problem for the next release.

@ashleybrener

This comment has been minimized.

Show comment
Hide comment
@ashleybrener

ashleybrener Dec 29, 2016

Contributor

@TimWillis I did same as you, this issue has been vexing me last few days

Contributor

ashleybrener commented Dec 29, 2016

@TimWillis I did same as you, this issue has been vexing me last few days

@ashleybrener

This comment has been minimized.

Show comment
Hide comment
@ashleybrener

ashleybrener Dec 30, 2016

Contributor

Here's the original commit 9e024dd#diff-d54b456d54b337c5989fdf67462089ae

@GianlucaGuarini what was the reason here for the show directive also evaluating the parent? Seems to cause some complications, removing this would also eliminate the need for my PR patch #2186

Contributor

ashleybrener commented Dec 30, 2016

Here's the original commit 9e024dd#diff-d54b456d54b337c5989fdf67462089ae

@GianlucaGuarini what was the reason here for the show directive also evaluating the parent? Seems to cause some complications, removing this would also eliminate the need for my PR patch #2186

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Jan 10, 2017

Member

Fixed in riot@3.0.6 now the show, hide if directives behave always consistently on any context

Member

GianlucaGuarini commented Jan 10, 2017

Fixed in riot@3.0.6 now the show, hide if directives behave always consistently on any context

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