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

Add {{macroMaybeModifier}} template macro #692

Closed
wants to merge 2 commits into from

Conversation

simonihmig
Copy link
Collaborator

Closes #680

@simonihmig simonihmig requested a review from ef4 February 13, 2021 16:21
@simonihmig simonihmig added the enhancement New feature or request label Feb 13, 2021
@simonihmig
Copy link
Collaborator Author

Just realized I should add some e2e tests in macro-tests. Will do that later...

Also no documentation yet, as we didn't have any for template macros yet, and I felt adding all of that was out of scope for this PR.

Also tests are failing for canary, but also for other PRs, so seems unrelated.

@simonihmig
Copy link
Collaborator Author

Just realized I should add some e2e tests in macro-tests. Will do that later...

Done!

Copy link
Contributor

@ef4 ef4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good.

@simonihmig simonihmig marked this pull request as draft March 4, 2021 17:04
@simonihmig
Copy link
Collaborator Author

I marked this PR as draft to not accidentally merge this prematurely, as we should probably first try out this alternative approach before introducing a new API here: #680 (comment)

simonihmig added a commit to simonihmig/embroider that referenced this pull request Mar 7, 2021
Previously the macro-if transform would already allow to switch between two modifier implementations with a `(if (macroCondition ...) foo bar)` sub-expression inside a modifier. This now also completely removes the modifier if the sub-expression statically evaluates to undefined, allowing you to conditionally apply modifiers. See embroider-build#680 (comment)

Closes embroider-build#680, supersedes embroider-build#692.

Note this only works with Ember 3.25, which now allows you to use a sub-expression as the modifier path, see embroider-build#680 (comment). Older versions would throw a parse error (not exactly sure at which version exactly the changes in the parser where introduced). Therefore I also had to replace the vendored template compiler (for tests) with that of Ember 3.25.
@rwjblue
Copy link
Collaborator

rwjblue commented Mar 8, 2021

Replaced by #712

@rwjblue rwjblue closed this Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

macroMaybeModifier needed?
3 participants