fix(platform-server): add styles to elements correctly #22527

CaerusKaru · 2018-03-01T15:30:08Z

Partially reverts fix(platform-server): generate correct stylings for camel case names #22263 due to lack of total spec complicance
on the server
Maintains the camel-case styles fix

CaerusKaru · 2018-03-01T15:31:12Z

vicb · 2018-03-01T21:18:03Z

What was the issue ? (could you please create a GH issue and link it from the test)
could you please rephrase the commit message:

fix(platform-server): <what does this fix ?>

partial revert of #22263

vicb · 2018-03-01T21:22:22Z

packages/platform-server/src/domino_adapter.ts

+    const styleMap = {};
+    const styleAttribute = element.getAttribute('style');
+    if (styleAttribute) {
+      const styleList = styleAttribute.split(/;+/g);


nit: what about /(\s*;\s*)+/g here ?

oh would not work either so maybe
1- split on /;+/g
2- trim() the parts
3- ignore empty parts

I changed the logic, let me know if that's what you had in mind

CaerusKaru · 2018-03-01T21:33:57Z

@vicb changed commit message and opened a new issue

agustinhaller · 2018-03-14T14:46:46Z

@vicb any updates on merging this PR?

vicb · 2018-03-14T19:29:00Z

packages/platform-server/src/domino_adapter.ts

+    if (styleAttribute) {
+      const styleList = styleAttribute.split(/;+/g);
+      for (let i = 0; i < styleList.length; i++) {
+        if (styleList[i].length > 0) {


triming before checking the length would probably make more sense ?

vicb · 2018-03-14T19:29:17Z

packages/platform-server/src/domino_adapter.ts

+          if (!style) {
+            continue;
+          }
+          const colon = style.indexOf(':');


nit colon -> colonIndex

vicb · 2018-03-14T19:30:32Z

packages/platform-server/src/domino_adapter.ts

+          if (colon === -1) {
+            throw new Error(`Invalid CSS style: ${style}`);
+          }
+          (styleMap as any)[style.substr(0, colon).trim()] = style.substr(colon + 1).trim();


nit: extract const name = style.substr(0, colon).trim();

there should be as any needed here

vicb · 2018-03-14T19:32:15Z

packages/platform-server/src/domino_adapter.ts

@@ -135,17 +135,53 @@ export class DominoAdapter extends BrowserDomAdapter {
    return href;
  }

+  /** @internal */
+  _readStyleAttribute(element: any) {
+    const styleMap = {};


const styleMap: {[name: string]: string} = {} and then remove un-needed "as any"

vicb · 2018-03-14T19:32:22Z

packages/platform-server/src/domino_adapter.ts

@@ -135,17 +135,53 @@ export class DominoAdapter extends BrowserDomAdapter {
    return href;
  }

+  /** @internal */
+  _readStyleAttribute(element: any) {


add return type

vicb · 2018-03-14T19:34:01Z

packages/platform-server/src/domino_adapter.ts

  setStyle(element: any, styleName: string, styleValue?: string|null) {
    styleName = styleName.replace(/([a-z])([A-Z])/g, '$1-$2').toLowerCase();
-    element.style[styleName] = styleValue;
+    const styleMap = this._readStyleAttribute(element);
+    (styleMap as any)[styleName] = styleValue;


no need for any if types are specified upstream

vicb · 2018-03-14T19:34:12Z

packages/platform-server/src/domino_adapter.ts

  }
  getStyle(element: any, styleName: string): string {
-    return element.style[styleName] || element.style.getPropertyValue(styleName);
+    const styleMap = this._readStyleAttribute(element);
+    return styleMap.hasOwnProperty(styleName) ? (styleMap as any)[styleName] : '';


no need for as any

vicb

a few nits but LGTM otherwise.
Thanks and sorry for the delay in reviewing

vicb · 2018-03-14T19:35:39Z

Typo in the commit message (complicance)
Was is the target: master and/or patch

CaerusKaru · 2018-03-14T20:10:56Z

Should be PR target: master & patch

vicb · 2018-03-14T20:14:42Z

packages/platform-server/src/domino_adapter.ts

  setStyle(element: any, styleName: string, styleValue?: string|null) {
    styleName = styleName.replace(/([a-z])([A-Z])/g, '$1-$2').toLowerCase();
-    element.style[styleName] = styleValue;
+    const styleMap = this._readStyleAttribute(element);
+    styleMap[styleName] = styleValue + '';


do we really want "undefined" as a possible value ?

vicb · 2018-03-14T20:15:24Z

packages/platform-server/src/domino_adapter.ts

  }
  getStyle(element: any, styleName: string): string {
-    return element.style[styleName] || element.style.getPropertyValue(styleName);
+    const styleMap = this._readStyleAttribute(element);
+    return styleMap.hasOwnProperty(styleName) ? styleMap[styleName] : '';


return styleMap[styleName] || ''; ?

* Partially reverts angular#22263 due to lack of total spec compliance on the server * Maintains the camel-case styles fix PR Close angular#22527

angular-automatic-lock-bot · 2019-09-13T10:22:48Z

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

googlebot added the cla: yes label Mar 1, 2018

CaerusKaru force-pushed the revert-22263 branch from 4947788 to c77b576 Compare March 1, 2018 15:30

CaerusKaru mentioned this pull request Mar 1, 2018

fix(platform-server): generate correct stylings for camel case names #22263

Closed

CaerusKaru force-pushed the revert-22263 branch from c77b576 to 7879a5d Compare March 1, 2018 15:38

CaerusKaru changed the title ~~chore: partial revert of #22263~~ fix(platform-server): partial revert of #22263 Mar 1, 2018

alexeagle requested a review from vikerman March 1, 2018 15:58

alexeagle added the area: server Issues related to server-side rendering label Mar 1, 2018

vicb added the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 1, 2018

vicb reviewed Mar 1, 2018

View reviewed changes

CaerusKaru changed the title ~~fix(platform-server): partial revert of #22263~~ fix(platform-server): add styles to elements correctly Mar 1, 2018

CaerusKaru force-pushed the revert-22263 branch from 7879a5d to fb9f9ec Compare March 1, 2018 21:33

agustinhaller mentioned this pull request Mar 13, 2018

Missing specified styles (both through [ngStyle] or using Renderer2 .setStyle in custom component angular/universal-starter#552

Closed

vicb reviewed Mar 14, 2018

View reviewed changes

vicb suggested changes Mar 14, 2018

View reviewed changes

CaerusKaru force-pushed the revert-22263 branch from fb9f9ec to 7be1a4a Compare March 14, 2018 20:10

vicb added the target: patch This PR is targeted for the next patch release label Mar 14, 2018

vicb reviewed Mar 14, 2018

View reviewed changes

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(platform-server): add styles to elements correctly #22527

fix(platform-server): add styles to elements correctly #22527

CaerusKaru commented Mar 1, 2018 •

edited

CaerusKaru commented Mar 1, 2018 •

edited

vicb commented Mar 1, 2018

vicb Mar 1, 2018 •

edited

vicb Mar 1, 2018

CaerusKaru Mar 1, 2018

CaerusKaru commented Mar 1, 2018

agustinhaller commented Mar 14, 2018

vicb Mar 14, 2018

CaerusKaru Mar 14, 2018

vicb Mar 14, 2018

CaerusKaru Mar 14, 2018

vicb Mar 14, 2018

vicb Mar 14, 2018

CaerusKaru Mar 14, 2018

vicb Mar 14, 2018

CaerusKaru Mar 14, 2018

vicb Mar 14, 2018

CaerusKaru Mar 14, 2018

vicb Mar 14, 2018

CaerusKaru Mar 14, 2018

vicb Mar 14, 2018

CaerusKaru Mar 14, 2018

vicb left a comment

vicb commented Mar 14, 2018

CaerusKaru commented Mar 14, 2018

vicb Mar 14, 2018

CaerusKaru Mar 14, 2018

vicb Mar 14, 2018

CaerusKaru Mar 14, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

fix(platform-server): add styles to elements correctly #22527

fix(platform-server): add styles to elements correctly #22527

Conversation

CaerusKaru commented Mar 1, 2018 • edited

CaerusKaru commented Mar 1, 2018 • edited

vicb commented Mar 1, 2018

vicb Mar 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaerusKaru commented Mar 1, 2018

agustinhaller commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb left a comment

Choose a reason for hiding this comment

vicb commented Mar 14, 2018

CaerusKaru commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 13, 2019

CaerusKaru commented Mar 1, 2018 •

edited

CaerusKaru commented Mar 1, 2018 •

edited

vicb Mar 1, 2018 •

edited