Skip to content

feat(ViewEncapsulation): default ViewEncapsulation to configurable - #7883

Closed
lacolaco wants to merge 1 commit into
angular:masterfrom
lacolaco:laco-default-view-encapsulation
Closed

lacolaco wants to merge 1 commit into
angular:masterfrom
lacolaco:laco-default-view-encapsulation

Conversation

@lacolaco

@lacolaco lacolaco commented Apr 2, 2016

Copy link
Copy Markdown
Contributor

New feature

  • What is the current behavior? (You can also link to an open issue here)

Default encapsulation is ViewEncapsulation.Emulated. this is not configurable.

  • What is the new behavior (if this is a feature change)?

Add a DI token; DEFAULT_VIEW_ENCAPSULATION to overwrite default encapsulation.

provide(DEFAULT_VIEW_ENCAPSULATION, {useValue: ViewEncapsulation.None})
  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No

  • Other information:

@lacolaco
lacolaco force-pushed the laco-default-view-encapsulation branch 2 times, most recently from fcdff03 to 807bf0c Compare April 2, 2016 23:34
@lacolaco

lacolaco commented Apr 3, 2016

Copy link
Copy Markdown
Contributor Author

I don't know a lot about Dart. CI is red by Dart.

@PatrickJS

Copy link
Copy Markdown
Contributor

this would be great for universal where the html component should default to none while the application should default to emulated

@alxhub alxhub added the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 8, 2016
@lacolaco

Copy link
Copy Markdown
Contributor Author

Conflictions are occurring by changes for code gen. I'll fix these

@lacolaco
lacolaco force-pushed the laco-default-view-encapsulation branch 6 times, most recently from 53f6c54 to af9cb75 Compare April 14, 2016 16:29
@lacolaco
lacolaco force-pushed the laco-default-view-encapsulation branch 3 times, most recently from ee5f955 to afd23c1 Compare May 20, 2016 04:43
@mhevery mhevery self-assigned this May 20, 2016
@mhevery

mhevery commented May 20, 2016

Copy link
Copy Markdown
Contributor

@laco0416 Could you rebase on the latest code and I will take a look.

@mhevery mhevery assigned tbosch and unassigned mhevery May 20, 2016
@mhevery

mhevery commented May 20, 2016

Copy link
Copy Markdown
Contributor

@tbosch can you have a look?

@lacolaco
lacolaco force-pushed the laco-default-view-encapsulation branch from afd23c1 to dd6aea1 Compare May 20, 2016 05:36
@lacolaco

Copy link
Copy Markdown
Contributor Author

@mhevery Of course! But there are some errors in the offline compiler test.

dist/all/@angular/compiler/test/offline_compiler_codegen_typed.ts(1,26): error TS2307: Cannot find module '@angular/compiler/test/offline_compiler_compa.css'.

I don't know how to fix it...

@lacolaco
lacolaco force-pushed the laco-default-view-encapsulation branch 2 times, most recently from 16bc8ad to d60344f Compare May 20, 2016 06:53
@lacolaco

lacolaco commented May 20, 2016 •

Copy link
Copy Markdown
Contributor Author

I think it is blocked by codegen() in compiler_cli/src/codegen.ts.

metadatas.forEach((metadata) => {
                let stylesheetPaths = metadata && metadata.template && metadata.template.styleUrls;
                if (stylesheetPaths) {
                  stylesheetPaths.forEach((path) => {
                    stylesheetPromises.push(this.generateStylesheet(
                        path, metadata.template.encapsulation === ViewEncapsulation.Emulated));
                  });
                }
              });
              return this.generateSource(metadatas);

In this scope, no injectors exist.
Should we find other ways to access to DEFAULT_VIEW_ENCAPSULATION ? (like global)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is no need to introduce a new ViewEncapsulation enum value. We should add a new field to CompilerConfig with the default ViewEncapsulation, read it and set it when calling new CompileTemplateMetadata in directive_normalizer.ts.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This also needs a test then in the directive_normalizer.ts

@tbosch tbosch added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label May 26, 2016
@lacolaco
lacolaco force-pushed the laco-default-view-encapsulation branch from d60344f to 9ad5f98 Compare May 26, 2016 15:07
@lacolaco
lacolaco force-pushed the laco-default-view-encapsulation branch from 9ad5f98 to ba91599 Compare May 26, 2016 15:10
BREAKING CHANGES:

DirectiveNormalizer takes new constructor arguments, `config:CompilerConfig`.
@lacolaco
lacolaco force-pushed the laco-default-view-encapsulation branch from ba91599 to cbb15ea Compare May 26, 2016 15:21
@lacolaco

Copy link
Copy Markdown
Contributor Author

@tbosch I've done. please review again!

@angular-automatic-lock-bot

Copy link
Copy Markdown

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 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer cla: yes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants