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

if activation #1256

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
9 participants
@rsbondi
Member

rsbondi commented Oct 7, 2015

#1020 - I hope I am understanding this issue correctly as no plunks were provided

http://plnkr.co/edit/ktJOiN?p=preview

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Oct 7, 2015

Member

cool! I can't wait to test it thank you!

Member

GianlucaGuarini commented Oct 7, 2015

cool! I can't wait to test it thank you!

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Oct 7, 2015

Member

Probably also parseExpressions should be blocked as well, and the child tags should not be initialized.
Check [this] http://plnkr.co/edit/YEfZoxaY6uFCMFLmhQPb?p=preview the placeholder image is loaded even when the tag is not visible

Member

GianlucaGuarini commented Oct 7, 2015

Probably also parseExpressions should be blocked as well, and the child tags should not be initialized.
Check [this] http://plnkr.co/edit/YEfZoxaY6uFCMFLmhQPb?p=preview the placeholder image is loaded even when the tag is not visible

@rsbondi

This comment has been minimized.

Show comment
Hide comment
@rsbondi

rsbondi Oct 7, 2015

Member

I will update this evening

Member

rsbondi commented Oct 7, 2015

I will update this evening

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Oct 7, 2015

Member

@rsbondi no stress! thank you

Member

GianlucaGuarini commented Oct 7, 2015

@rsbondi no stress! thank you

@rsbondi

This comment has been minimized.

Show comment
Hide comment
@rsbondi

rsbondi Oct 18, 2015

Member

Ok, I am stuck on this one, I don't want to hold up the release, what is here is an improvement and could go with the release. It is not as simple as deferring the parsing, once the dom is created, it is too late and the decision to parse or not comes after the dom is created. I have been attempting a replace of src="..." with riot-src="...". and try to swap back on update(), but somewhere along the way, the attribute is getting lost, I have been unable to track it down. So, please consider releasing with this fix only, while I continue to track this down.

Member

rsbondi commented Oct 18, 2015

Ok, I am stuck on this one, I don't want to hold up the release, what is here is an improvement and could go with the release. It is not as simple as deferring the parsing, once the dom is created, it is too late and the decision to parse or not comes after the dom is created. I have been attempting a replace of src="..." with riot-src="...". and try to swap back on update(), but somewhere along the way, the attribute is getting lost, I have been unable to track it down. So, please consider releasing with this fix only, while I continue to track this down.

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Oct 19, 2015

Member

@rsbondi Ok so maybe we could publish this update with riot 2.3.1 don't worry ;)

Member

GianlucaGuarini commented Oct 19, 2015

@rsbondi Ok so maybe we could publish this update with riot 2.3.1 don't worry ;)

@oderwat

This comment has been minimized.

Show comment
Hide comment
@oderwat

oderwat Oct 21, 2015

UPDATE 2: I think we did something really stupid but it also seems related to stuff getting parsed even if the result is not needed. We probably found a solution for our special problem though by using custom sub-tags.


Consider following as "outdated":

I think we run into this problem (performance wise). I just locally merged the PR into dev and build a test version. We are going to test if this is the culprit. Basically we have a lot of "inactive" code with forms in many rows containing select/option which seem to be updated all the time when scrolling the containing div (?) but for real just one of them will ever be shown (if you edit a row). Not sure if it is related but I found a lot of "Render HTML" calls and all of them contain "<option...." where not one of them actually is visible.

UPDATE: Our original problem is not "fixed" with the current PR. But we could validate that the speed problem has to do with all those "hidden" select/option elements. I am not sure if that is related to this PRs intention or just bad implementation by us which can be changed now as reorderin is in place.

oderwat commented Oct 21, 2015

UPDATE 2: I think we did something really stupid but it also seems related to stuff getting parsed even if the result is not needed. We probably found a solution for our special problem though by using custom sub-tags.


Consider following as "outdated":

I think we run into this problem (performance wise). I just locally merged the PR into dev and build a test version. We are going to test if this is the culprit. Basically we have a lot of "inactive" code with forms in many rows containing select/option which seem to be updated all the time when scrolling the containing div (?) but for real just one of them will ever be shown (if you edit a row). Not sure if it is related but I found a lot of "Render HTML" calls and all of them contain "<option...." where not one of them actually is visible.

UPDATE: Our original problem is not "fixed" with the current PR. But we could validate that the speed problem has to do with all those "hidden" select/option elements. I am not sure if that is related to this PRs intention or just bad implementation by us which can be changed now as reorderin is in place.

@GianlucaGuarini GianlucaGuarini referenced this pull request Nov 8, 2015

Closed

Riot next - roadmap to riot 2.4.0 #1322

1 of 9 tasks complete
@aMarCruz

This comment has been minimized.

Show comment
Hide comment
@aMarCruz

aMarCruz Nov 13, 2015

Member

@rsbondi , can help some if detection with the compiler?

Member

aMarCruz commented Nov 13, 2015

@rsbondi , can help some if detection with the compiler?

@rsbondi

This comment has been minimized.

Show comment
Hide comment
@rsbondi

rsbondi Nov 13, 2015

Member

@aMarCruz I am thinking that it would be helpful, not sure what path to go, this suggestion or some other. There is also potentially this.

Member

rsbondi commented Nov 13, 2015

@aMarCruz I am thinking that it would be helpful, not sure what path to go, this suggestion or some other. There is also potentially this.

@aMarCruz

This comment has been minimized.

Show comment
Hide comment
@aMarCruz

aMarCruz Nov 13, 2015

Member

@rsbondi , he comments solution is complicate.

Member

aMarCruz commented Nov 13, 2015

@rsbondi , he comments solution is complicate.

@avimar

This comment has been minimized.

Show comment
Hide comment
@avimar

avimar Dec 17, 2015

Whoa, just saw this: an image tag with an 'if' loads the image even if it evaluates to false, it just doesn't show it.

avimar commented Dec 17, 2015

Whoa, just saw this: an image tag with an 'if' loads the image even if it evaluates to false, it just doesn't show it.

@nippur72

This comment has been minimized.

Show comment
Hide comment
@nippur72

nippur72 Dec 22, 2015

Contributor

what's the status of this PR ? is it working or it's on a dead end?

Contributor

nippur72 commented Dec 22, 2015

what's the status of this PR ? is it working or it's on a dead end?

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Dec 22, 2015

Member

@nippur72 it needs to be updated and it's on the top of our prio list.. @rsbondi is working on it but I will try/propose my solution after the Christmas holidays

Member

GianlucaGuarini commented Dec 22, 2015

@nippur72 it needs to be updated and it's on the top of our prio list.. @rsbondi is working on it but I will try/propose my solution after the Christmas holidays

@rsbondi rsbondi referenced this pull request Dec 23, 2015

Closed

re-submit if-activation #1481

@rsbondi rsbondi closed this Dec 23, 2015

@thinkloop

This comment has been minimized.

Show comment
Hide comment
@thinkloop

thinkloop Dec 28, 2015

May want to update the guide talking about this issue if it is fully resolved:

IMPORTANT Using conditionals attributes on custom nested tags does not stop riot from evaluating the hidden expressions - we are working on a patch to solve this issue

thinkloop commented Dec 28, 2015

May want to update the guide talking about this issue if it is fully resolved:

IMPORTANT Using conditionals attributes on custom nested tags does not stop riot from evaluating the hidden expressions - we are working on a patch to solve this issue

@osv

This comment has been minimized.

Show comment
Hide comment
@osv

osv Feb 12, 2016

+1, update guide, I come here from guild-line :)

osv commented Feb 12, 2016

+1, update guide, I come here from guild-line :)

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Feb 12, 2016

Member

@osv this behavior has been already updated in riot@next in the next major riot release this issue will disappear ✌️

Member

GianlucaGuarini commented Feb 12, 2016

@osv this behavior has been already updated in riot@next in the next major riot release this issue will disappear ✌️

@osv

This comment has been minimized.

Show comment
Hide comment
@osv

osv Feb 16, 2016

oh, nice

osv commented Feb 16, 2016

oh, nice

@etorrenti

This comment has been minimized.

Show comment
Hide comment
@etorrenti

etorrenti Apr 8, 2017

@GianlucaGuarini: this behavior has been already updated in riot@next in the next major riot release this issue will disappear ✌️

Any chance of seeing this fixed in riot 2.x? I'd be glad to migrate to v3, but cannot at the moment.
Any known workarounds?

Thanks,
E.

etorrenti commented Apr 8, 2017

@GianlucaGuarini: this behavior has been already updated in riot@next in the next major riot release this issue will disappear ✌️

Any chance of seeing this fixed in riot 2.x? I'd be glad to migrate to v3, but cannot at the moment.
Any known workarounds?

Thanks,
E.

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Apr 8, 2017

Member

no this will never land in riot 2 because it's a breaking change

Member

GianlucaGuarini commented Apr 8, 2017

no this will never land in riot 2 because it's a breaking change

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