feat(components): add support for CSS encapsulation to transcluding components #1315

vsavkin · 2014-08-05T15:15:31Z

This PR depends on #1293.

Extract ComponentCssLoader so it can be shared by ShadowDomComponentFactory and TranscludingComponentFactory
Implement a CSS transformer similar to the one provided by Platform.JS.
Rename EventHandler into ShadowBoundary
Rename WebPlatform into WebPlatformShim and provide two implementations: one based on platform.js and the other one built into Angular.

TODO:

Refactor ShadowDomComponentFactory#call and TranscludingComponentFactory#call.
Test ShimmingViewCache in isolation. At the moment it is tested indirectly.
Test the default shim

Includes #1304

vicb · 2014-08-05T16:24:45Z

lib/core_dom/css_shim.dart

+  bool get isBody => true;
+}
+
+class _Lexer {


could csslib help here ?

I'll investigate if it can be used.

It could be used, but since the library is generic and does more than we need, importing it adds 160Kb to the generated (and minified) JS file. So we should probably try to build something more tailored to our use case.

I would have think that the tree shaking would have been more effective...
I think we can forget it then

vicb · 2014-08-05T16:31:44Z

@vsavkin IIUC this PR fixes #1300 & includes #1304, could you confirm ?

Could you also please what is left to do (as this is a WIP) to ease the review ?

Very good job at first look !

vicb · 2014-08-05T16:32:31Z

What about #1305, do you think it could be added ?

vsavkin · 2014-08-05T16:56:28Z

@vicb #1300 is next on my list. It will be a part of this PR. #1304 has not been merged into master yet, so it is not included. I have not thought about #1305 much, so I need to look into it more.

vicb · 2014-08-05T17:16:53Z

In think you already have solved #1304, that was the issue with the cache keys (not depending on the selector)

vicb · 2014-08-06T14:23:57Z

lib/core_dom/web_platform_shim.dart

+abstract class WebPlatformShim {
+  /**
+   * Because this code uses `strictStyling` for the polymer css shim, it is required to add the
+   * custom element’s name as an attribute on all DOM nodes in the shadowRoot (e.g. <span x-foo>).


@vsavkin this comment should probably be updated ?

vsavkin · 2014-08-06T14:27:57Z

@vicb you are totally right about #1304. It is included into this PR.

Talking of #1305. I've added unit tests for ShimmingViewCache and DefaultPlatformShim, but not for PlatformJsBasedShim. The main responsibility of PlatformJsBasedShim is to delegate to Platform.js, so I'm not sure how much value we would get by isolating it from platform.js. To do it we would have to wrap platform.js in another object, register it with DI, and then pass it to PlatformJsBasedShim. Not sure if all this is necessary, considering we would have to have some integration tests (or e2e tests) anyways. What do you think?

vicb · 2014-08-06T14:28:25Z

test/core_dom/compiler_spec.dart

@@ -53,7 +53,8 @@ void main() {
    beforeEachModule((Module m) {
      return m
          ..bind(ComponentFactory, toImplementation: TranscludingComponentFactory)
-          ..bind(SimpleComponent);
+          ..bind(SimpleComponent)
+          ..bind(DefaultPlatformShim, toImplementation: DefaultPlatformShim);


bind(DefaultPlatformShim); ?

vicb · 2014-08-07T07:21:47Z

I would prefer to see some unit tests, e2e tests only test for whatever
platform they're executed on.

It should not be that complex by using the way I've suggested in the PR:

create mocks for ShadowCss and Platform, assign the later to
js.context['Platform'] beforeEach tests
check that the expected methods are called on the mock according to
the config (shim / native)
restore the initial js.context['Platform'] afterEach test

If you can come something simple like this, it's probably worth it. If
it gets too complex e2e might be enough.

On 08/06/2014 04:27 PM, Victor Savkin wrote:

@vicb https://github.com/vicb you are totally right about #1304
#1304. It is included
into this PR.

Talking of #1305
#1305. I've added unit
tests for ShimmingViewCache and DefaultPlatformShim, but not for
PlatformJsBasedShim. The main responsibility of PlatformJsBasedShim is
to delegate to Platform.js, so I'm not sure how much value we would
get by isolating it from platform.js. To do it we would have to wrap
platform.js in another object, register it with DI, and then pass it
to PlatformJsBasedShim. Not sure if all this is necessary, considering
we would have to have some integration tests (or e2e tests) anyways.
What do you think?

—
Reply to this email directly or view it on GitHub
#1315 (comment).

vsavkin · 2014-08-07T12:53:22Z

@vicb I'm not sure if we should mock the Platform object. Doing so breaks "Don't Mock What You Don't Own". I'd prefer to wrap the Platform object into something we control and mock that.

vicb · 2014-08-08T10:08:05Z

lib/core_dom/component_css_loader.dart

+
+  async.Future<String> _fetch(String cssUrl) {
+    return http.get(cssUrl, cache: templateCache)
+        .then((resp) => resp.responseText, onError: (e) => '/*\n$e\n*/\n');


Could you please remove the \n from here, they make testing with IE fails, see https://github.com/angular/angular.dart/pull/1288/files#diff-184d55c4be9ba89ff317b308735c6ae9R91. You would also need to update the corresponding test.

I think is it much easier to include this here are this will not be updated while merging the PRs to master as the file has changed.

vsavkin · 2014-08-30T00:26:28Z

@mhevery is it "review" or "rebase"?

…ents * Extract ComponentCssLoader so it can be shared by ShadowDomComponentFactory and TranscludingComponentFactory * Provide a built-in implementation of WebPlatformShim using css_shim * Implement a CSS transformer similar to the one provided by Platform.JS * Rename EventHandler into ShadowBoundary

mhevery added cla: yes and removed cla: no labels Aug 5, 2014

vicb reviewed Aug 5, 2014
View reviewed changes

vicb changed the title ~~Add support for CSS encapsulation to transcluding components~~ [WIP] Add support for CSS encapsulation to transcluding components Aug 5, 2014

vsavkin mentioned this pull request Aug 6, 2014

WebPlatform bug ? #1300

Open

vicb reviewed Aug 6, 2014
View reviewed changes

vsavkin changed the title ~~[WIP] Add support for CSS encapsulation to transcluding components~~ feat(components): add support for CSS encapsulation to transcluding components Aug 6, 2014

mhevery added api: new labels Aug 7, 2014

vicb reviewed Aug 8, 2014
View reviewed changes

mhevery removed the status: Travis OK label Aug 28, 2014

vsavkin force-pushed the add_css_encapsulation branch from 585fafe to cc9e973 Compare August 29, 2014 12:07

mhevery added the status: Travis OK label Aug 29, 2014

vsavkin added action: merge and removed action: g3-conflict labels Aug 29, 2014

mhevery removed the action: merge label Aug 29, 2014

vsavkin force-pushed the add_css_encapsulation branch from cc9e973 to 22d4294 Compare August 29, 2014 21:42

mhevery added action: review and removed status: Travis OK labels Aug 29, 2014

mhevery added status: LGTM and removed action: review labels Sep 2, 2014

vsavkin force-pushed the add_css_encapsulation branch from 22d4294 to 78175c4 Compare September 2, 2014 17:02

mhevery removed the action: merge label Sep 2, 2014

vsavkin force-pushed the add_css_encapsulation branch 2 times, most recently from 3b174c0 to d616410 Compare September 3, 2014 13:42

vsavkin added the status: Travis OK label Sep 3, 2014

mhevery added the action: merge label Sep 3, 2014

vsavkin mentioned this pull request Sep 3, 2014

Change ShadowBoundary to ignore duplicate styles #1423

Merged

chirayuk force-pushed the master branch 2 times, most recently from 8fd235c to 50e2645 Compare September 5, 2014 23:10

vsavkin force-pushed the add_css_encapsulation branch 2 times, most recently from 1ba3218 to ad194a0 Compare September 10, 2014 16:28

mhevery removed status: Travis OK action: merge labels Sep 10, 2014

vsavkin force-pushed the add_css_encapsulation branch from ad194a0 to 18843e1 Compare September 11, 2014 16:25

vsavkin merged commit 18843e1 into dart-archive:master Sep 11, 2014

vicb mentioned this pull request Jan 23, 2015

refactor(WebPlatform): remove deprecated code #1655

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(components): add support for CSS encapsulation to transcluding components #1315

feat(components): add support for CSS encapsulation to transcluding components #1315

vsavkin commented Aug 5, 2014

vicb Aug 5, 2014

vsavkin Aug 5, 2014

vsavkin Aug 5, 2014

vicb Aug 5, 2014

vicb commented Aug 5, 2014

vicb commented Aug 5, 2014

vsavkin commented Aug 5, 2014

vicb commented Aug 5, 2014

vicb Aug 6, 2014

vsavkin commented Aug 6, 2014

vicb Aug 6, 2014

vicb commented Aug 7, 2014

vsavkin commented Aug 7, 2014

vicb Aug 8, 2014

vsavkin commented Aug 30, 2014

feat(components): add support for CSS encapsulation to transcluding components #1315

feat(components): add support for CSS encapsulation to transcluding components #1315

Conversation

vsavkin commented Aug 5, 2014

vicb Aug 5, 2014

Choose a reason for hiding this comment

vsavkin Aug 5, 2014

Choose a reason for hiding this comment

vsavkin Aug 5, 2014

Choose a reason for hiding this comment

vicb Aug 5, 2014

Choose a reason for hiding this comment

vicb commented Aug 5, 2014

vicb commented Aug 5, 2014

vsavkin commented Aug 5, 2014

vicb commented Aug 5, 2014

vicb Aug 6, 2014

Choose a reason for hiding this comment

vsavkin commented Aug 6, 2014

vicb Aug 6, 2014

Choose a reason for hiding this comment

vicb commented Aug 7, 2014

vsavkin commented Aug 7, 2014

vicb Aug 8, 2014

Choose a reason for hiding this comment

vsavkin commented Aug 30, 2014