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

fix(animations): set animations styles properly on platform-server #24624

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@vikerman
Contributor

vikerman commented Jun 22, 2018

Animations styles weren't getting properly set on platform-server because of erroneous checks and absence of reflection of style property to attribute on the server.

The fix corrects the check for platform and explicitly reflects the style property to the attribute.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Styles for animations weren't getting set properly on platform-server.

What is the new behavior?

Sets the style properly.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@vikerman vikerman requested a review from matsko Jun 22, 2018

@googlebot googlebot added the cla: yes label Jun 22, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Jun 22, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Jun 22, 2018

@vikerman vikerman requested a review from jasonaden Jun 22, 2018

@@ -125,12 +126,40 @@ export function copyStyles(
return destination;
}
function getKeyValue(element: any, key: string) {
const dashKey = key.replace(/([a-z])([A-Z])/g, '$1-$2').toLowerCase();
const newValue = element.style[key];

This comment has been minimized.

@matsko

matsko Jun 22, 2018

Member

dashed-key values don't work consistently among browsers using element.style. Use style.getPropertyValue instead.

This comment has been minimized.

@vikerman

vikerman Jun 22, 2018

Contributor

This is used only on the server to write to the style attribute. We don't use dashKey to read back the value.

@@ -125,12 +126,40 @@ export function copyStyles(
return destination;
}
function getKeyValue(element: any, key: string) {

This comment has been minimized.

@matsko

matsko Jun 22, 2018

Member

This function is a bit hard to understand what exactly it does given how general the name is. Add a description please.

This comment has been minimized.

@vikerman

vikerman Jun 22, 2018

Contributor

Done. Renamed the function and added comments.

for (let i = 0; i < element.style.length; i++) {
styleAttrValue += getKeyValue(element, element.style.item(i));
}
for (const key in element.style) {

This comment has been minimized.

@matsko

matsko Jun 22, 2018

Member

store an array of keys in the loop above and then forEach over them here (since for in will include all the prototypical stuff in the loop as well).

This comment has been minimized.

@vikerman

vikerman Jun 22, 2018

Contributor

There are two kinds of style properties that need to be reflected.

Things that Domino understand we can enumerate through element.style.item(i). Things that Domino don't understand(like the 'transform') we have to just use the Object property and use the [] accessor to get their values. The common part is creating the 'key:value' string which has been put into getStyleAttributeString.

}
for (const key in element.style) {
if (key.startsWith('_')) {
continue;

This comment has been minimized.

@matsko

matsko Jun 22, 2018

Member

Add a comment here.

}
}
function writeStyleAttribute(element: any) {

This comment has been minimized.

@matsko

matsko Jun 22, 2018

Member

Explain here as well what's so special about this method since it doesn't take in any additional params.

This comment has been minimized.

@vikerman

vikerman Jun 22, 2018

Contributor

Added comments.

@mary-poppins

This comment has been minimized.

mary-poppins commented Jun 22, 2018

fix(animations): set animations styles properly on platform-server
Animations styles weren't getting properly set on platform-server because of erroneous checks and absence of reflection of style property to attribute on the server.

The fix corrects the check for platform and explicitly reflects the style property to the attribute.
@matsko

matsko approved these changes Jun 22, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Jun 22, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Jun 22, 2018

@mhevery mhevery closed this in 6e20e0a Jun 25, 2018

mhevery added a commit that referenced this pull request Jun 25, 2018

fix(animations): set animations styles properly on platform-server (#…
…24624)

Animations styles weren't getting properly set on platform-server because of erroneous checks and absence of reflection of style property to attribute on the server.

The fix corrects the check for platform and explicitly reflects the style property to the attribute.

PR Close #24624
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment