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

Else CustomAttrbiute not having bindingContext set #322

Closed
fragsalat opened this Issue Oct 9, 2017 · 5 comments

Comments

Projects
None yet
5 participants
@fragsalat
Contributor

fragsalat commented Oct 9, 2017

I'm submitting a bug report

  • Library Version:
    1.5.1

Please tell us about your environment:

  • Operating System:
    Linux (Ubuntu 17.10)

  • Node Version:
    8.4.0

  • NPM Version:
    5.4.2

  • Browser:
    all

  • Language:
    ESNext

Current behavior:
The element having the else attribute has no binding context which leads to missing data when the initial condition is false.
Example:
https://gist.run/?id=371fa4bf53068f0dd4463684b165cb66

Cause
The cause for this problem might be that the If.conditionChanged() function is called when the if is bound. Since the if attribute appears mostly before the else attribute, the else attribute has no bindingContext yet.
https://github.com/aurelia/templating-resources/blob/master/src/if.js#L23

Possible solution
Call the conditionChanged function also when else is bound or just use the binding context of the if attribute.

@jdanyow jdanyow added the bug label Oct 10, 2017

@fragsalat

This comment has been minimized.

Show comment
Hide comment
@fragsalat

fragsalat Oct 10, 2017

Contributor

Do you need support with this? I mean I can fix this but do you have time to review, merge, test and release the code?

Contributor

fragsalat commented Oct 10, 2017

Do you need support with this? I mean I can fix this but do you have time to review, merge, test and release the code?

@jdanyow

This comment has been minimized.

Show comment
Hide comment
@jdanyow

jdanyow Oct 11, 2017

Member

@fragsalat we'd definitely take a PR for this

Member

jdanyow commented Oct 11, 2017

@fragsalat we'd definitely take a PR for this

@huochunpeng

This comment has been minimized.

Show comment
Hide comment
@huochunpeng

huochunpeng Oct 11, 2017

Contributor

I got exactly the same issue just now when I try the new else binding :-(
Just composed an almost identical gist-run.

Having additional binding inside a branch is a common use case. The else binding is a neat new feature, but it looks like it needs more test coverage before I can trust it in production.

Contributor

huochunpeng commented Oct 11, 2017

I got exactly the same issue just now when I try the new else binding :-(
Just composed an almost identical gist-run.

Having additional binding inside a branch is a common use case. The else binding is a neat new feature, but it looks like it needs more test coverage before I can trust it in production.

@fragsalat

This comment has been minimized.

Show comment
Hide comment
@fragsalat

fragsalat Oct 11, 2017

Contributor

@huochunpeng I fixed this now and it just has to get merged and released :)

PS: @jdanyow Could you may be internally make some pressure for my aurelia-virtualization PR? :)
It's open for 15 days already and the responsible person seem to have no more time for this project anymore :(

Contributor

fragsalat commented Oct 11, 2017

@huochunpeng I fixed this now and it just has to get merged and released :)

PS: @jdanyow Could you may be internally make some pressure for my aurelia-virtualization PR? :)
It's open for 15 days already and the responsible person seem to have no more time for this project anymore :(

@jods4

This comment has been minimized.

Show comment
Hide comment
@jods4

jods4 Nov 1, 2017

Contributor

This should be fixed in the current release, I think we can close this issue?
@EisenbergEffect

Contributor

jods4 commented Nov 1, 2017

This should be fixed in the current release, I think we can close this issue?
@EisenbergEffect

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