-
Notifications
You must be signed in to change notification settings - Fork 33
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
Fix <LinkTo> usage with class
attribute.
#82
Conversation
@Alonski It seems you are running into the same linter issue as me a few times: CI runs with the latest dependencies, where prettier rules are slightly different, so what's passing locally is failing in CI. You would have to I just created #84 to fix this... |
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 @Alonski for working on this! I said yesterday that I would try to fix it today, but you seem to already have found the problem, so I'll leave it up to you to finish the PR, unless you want me help! :)
lib/ast-link-to-transform.js
Outdated
if (attributeValue.params.length) { | ||
// Deals with helper usage | ||
return b.path( | ||
b.sexpr(attributeValue.path.original, attributeValue.params, null, attributeValue.loc) |
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.
You are not passing hash
here. But yes, this is the culprit, my initial implementation was too naive.
Also I realized the original generic transform for angle brackets also has to deal with transforming attributes obviously, and has this figured out properly already: https://github.com/rwjblue/ember-angle-bracket-invocation-polyfill/blob/master/lib/ast-transform.js#L40-L53.
So I think it would be best to extract this into another helper function, and reuse it across all three transforms (including the one for <Input>
, which suffers from the same problem I think).
@simonihmig Ready 4 Review :) Thanks! |
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.
Looks good! Can you:
- rebase and run eslint fix (I think that will fix some of the issues with formatting in CI)
- add another test for
<LinkTo @route=“bar” class=“ma{{“in”}}”></LinkTo>
(to confirm thatConcatStatement
s are handled right)
Thank you!
class
attribute.
Using a helper in a class property doesn't render the class in the rendered HTML.
Example:
Expected HTML:
Actual:
Fixes: #83