-
Notifications
You must be signed in to change notification settings - Fork 137
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
Add support for {{unless}}
to the macroCondition
macro
#1858
Add support for {{unless}}
to the macroCondition
macro
#1858
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks nice.
32374bd
to
a1310c8
Compare
@ef4 is there anything that needs to happen before this can be merged or did this simply fall through the cracks 😄? |
@Windvis one thing that might be slowing this down is that you're targeting main with this change 🤔 are you intending to make use of this soon? |
@mansona No, there is no rush since it's easy to work around the issue. If I target stable, how does the change get back into main? Does it get cherry picked, or do I need to open a second PR that targets main? |
@Windvis once things are released in stable we forward-port them to main. It tends to be a lot easier to do it this way, also since we're keeping things up to date often enough 👍 Essentially you would not need to do anything extra, it would be on us :) |
Let's try to retarget this to |
a1310c8
to
6d2baff
Compare
This bug was lurking since #1858 but it didn't actually cause a test failure until an updated glimmer-vm landed. Not entirely sure why that was, but the bug is definitely real and this is the appropriate fix.
I noticed the
macroCondition
macro could only be used with the{{if}}
helper / keyword. This change makes it possible to use{{unless}}
as well.(Not a very DRY change, but it seemed better to keep things simple and duplicate the if version 😄.)