Skip to content
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

Merged
merged 12 commits into from
Aug 6, 2019

Conversation

Alonski
Copy link
Contributor

@Alonski Alonski commented Aug 4, 2019

Using a helper in a class property doesn't render the class in the rendered HTML.

Example:

<LinkTo @route="foo" class={{concat "main"}}>Link</LinkTo>

Expected HTML:

<a href="/foo" class="main ember-view">Link</a>

Actual:

<a href="/foo" class="ember-view">Link</a>

Fixes: #83

@simonihmig
Copy link
Contributor

@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 yarn --no-lockfile and yarn lint:js --fix locally to fix the linting error that is failing the build in CI!

I just created #84 to fix this...

Copy link
Contributor

@simonihmig simonihmig left a 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! :)

if (attributeValue.params.length) {
// Deals with helper usage
return b.path(
b.sexpr(attributeValue.path.original, attributeValue.params, null, attributeValue.loc)
Copy link
Contributor

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).

@Alonski
Copy link
Contributor Author

Alonski commented Aug 6, 2019

@simonihmig Ready 4 Review :) Thanks!

Copy link
Member

@rwjblue rwjblue left a 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 that ConcatStatements are handled right)

Thank you!

@rwjblue rwjblue changed the title feat: Add failing <LinkTo> test with dynamic class Fix <LinkTo> usage with class attribute. Aug 6, 2019
@rwjblue rwjblue added the bug Something isn't working label Aug 6, 2019
@rwjblue rwjblue merged commit dc65871 into ember-polyfills:master Aug 6, 2019
@Alonski Alonski deleted the bugfix/dynamic-class branch December 25, 2019 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic class usage in <LinkTo> fails
3 participants