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

feat(ivy): support inline <style> and <link> tags in components #28997

Closed
wants to merge 1 commit into from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Feb 27, 2019

Angular supports using <style> and tags inline in component
templates, but previously such tags were not implemented within the ngtsc
compiler. This commit introduces that support.

@alxhub alxhub requested a review from a team as a code owner February 27, 2019 00:47
@ngbot ngbot bot added this to the needsTriage milestone Feb 27, 2019
@marclaval
Copy link
Contributor

As part of this PR, could you please enable the component-styles e2e tests in https://github.com/angular/angular/blob/master/aio/tools/examples/run-example-e2e.js ?

Angular supports using <style> and <link> tags inline in component
templates, but previously such tags were not implemented within the ngtsc
compiler. This commit introduces that support.

FW-1069 #resolve
@@ -319,7 +317,10 @@ function getE2eSpecs(basePath, filter) {
// Find all e2e specs in a given example folder.
function getE2eSpecsFor(basePath, specFile, filter) {
// Only get spec file at the example root.
// The formatter doesn't understand nested template string expressions (honestly, neither do I).
// clang-format off
Copy link
Member

Choose a reason for hiding this comment

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

🤣

if (ts.isStringLiteral(templateExpr) || ts.isNoSubstitutionTemplateLiteral(templateExpr)) {
// the start and end of the `templateExpr` node includes the quotation marks, which we
// must
// strip
Copy link
Member

Choose a reason for hiding this comment

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

O.P. formatter

const jsContents = env.getContents('test.js');
expect(jsContents).toContain('styles: ["h1[_ngcontent-%COMP%] {font-size: larger}"]');
});
});
Copy link
Member

Choose a reason for hiding this comment

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

Should there be similar tests for templateUrl components?

Copy link
Member Author

Choose a reason for hiding this comment

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

We already have tests that verify templateUrl works correctly. The logic is such that the origin of the template doesn't matter by the time we extract <style> and <link>.

Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

LGTM

@alxhub alxhub added the target: major This PR is targeted for the next major release label Feb 27, 2019
@alxhub alxhub removed the request for review from AndrewKushnir February 27, 2019 19:28
@alxhub alxhub added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Feb 27, 2019
@alxhub
Copy link
Member Author

alxhub commented Feb 27, 2019

Caretaker: I am the approver for fw-compiler, so this is good to go.

@benlesh benlesh closed this in 827e89c Feb 27, 2019
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants