The Wayback Machine - https://web.archive.org/web/20210905001716/https://github.com/angular/angular/pull/42763
Skip to content

/ angular

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

Jump to bottom

fix(animations): normalize final styles in buildStyles #42763

Closed
wants to merge 4 commits into from
Closed

fix(animations): normalize final styles in buildStyles #42763

wants to merge 4 commits into from
+68 −27

Conversation

@dario-piotrowicz
Copy link
Contributor

@dario-piotrowicz dario-piotrowicz commented Jul 4, 2021 •
edited

the final styles created in buildStyles lack normalization, meaning that pixel values remain as numbers (without "px") and so such properties fail to be correctly set/applied

Example: "width: 300" would be applied as "width": "300" (and thus ignored) instead of the correct "width": "300px"

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • 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?

At the end of animations numerical values are not normalized thus they are not saved correctly

Example with width, notice how the animation widths are ignored at the end:
width-before

stackblitz

Issue Number: #24698

What is the new behavior?

numerical values get normalized at animations end thus fixing the issue

Example after fix:
width-after

Does this PR introduce a breaking change?

  • Yes
  • No
@google-cla google-cla bot added the cla: yes label Jul 4, 2021
@pullapprove pullapprove bot requested a review from IgorMinar Jul 4, 2021
@JoostK
JoostK reviewed Jul 4, 2021
View changes
Copy link
Member

@JoostK JoostK left a comment

Heya, thank you for taking a look at this. The animations package is not a simple one to get your hands dirty on.

It would be great if this could have a test. I think the best place for such a test would be in the integration suite located in packages/core/test/animation. I am also a bit concerned about the normalizer being optional.
packages/animations/browser/src/util.ts Outdated Show resolved Hide resolved
@ngbot ngbot bot added this to the Backlog milestone Jul 4, 2021
@dario-piotrowicz
dario-piotrowicz reviewed Jul 4, 2021
View changes
packages/animations/browser/src/util.ts Outdated Show resolved Hide resolved
@dario-piotrowicz
dario-piotrowicz reviewed Jul 4, 2021
View changes
packages/animations/browser/src/util.ts Outdated Show resolved Hide resolved
@dario-piotrowicz
Copy link
Contributor Author

@dario-piotrowicz dario-piotrowicz commented Jul 4, 2021

Heya, thank you for taking a look at this. The animations package is not a simple one to get your hands dirty on.

It would be great if this could have a test. I think the best place for such a test would be in the integration suite located in packages/core/test/animation. I am also a bit concerned about the normalizer being optional.

Thank you very much!!! 😃

About the test, consider it done! 😉

Also the normalizer should definitely not be optional! you're totally right! I'm going to amend it 🙂

@dario-piotrowicz dario-piotrowicz force-pushed the dario-piotrowicz:animations-normalize-toStyles branch 2 times, most recently from f876e92 to 722a99a Jul 4, 2021
@dario-piotrowicz
dario-piotrowicz reviewed Jul 4, 2021
View changes
packages/animations/browser/src/util.ts Outdated Show resolved Hide resolved
@dario-piotrowicz dario-piotrowicz changed the title fix(animations): normalize toStyles on animation player onDestroy fix(animations): normalize styles on animation player setStyles Jul 4, 2021
@dario-piotrowicz dario-piotrowicz force-pushed the dario-piotrowicz:animations-normalize-toStyles branch 2 times, most recently from 42d655a to 4cd0367 Jul 4, 2021
@pullapprove pullapprove bot requested a review from alxhub Jul 4, 2021
@dario-piotrowicz
dario-piotrowicz reviewed Jul 4, 2021
View changes
...s/core/test/animation/animations_with_web_animations_integration_spec.ts Show resolved Hide resolved
@dario-piotrowicz
dario-piotrowicz reviewed Jul 4, 2021
View changes
packages/animations/browser/src/util.ts Outdated Show resolved Hide resolved
@dario-piotrowicz dario-piotrowicz requested a review from JoostK Jul 4, 2021
@dario-piotrowicz dario-piotrowicz changed the title fix(animations): normalize styles on animation player setStyles fix(animations): normalize styles in animation player setStyles Jul 4, 2021
@JoostK
JoostK reviewed Jul 4, 2021
View changes
Copy link
Member

@JoostK JoostK left a comment

I think this looks good now. I'm marking for presubmit to see how this behaves in Google.
packages/animations/browser/src/util.ts Outdated Show resolved Hide resolved
...s/core/test/animation/animations_with_web_animations_integration_spec.ts Show resolved Hide resolved
packages/animations/browser/src/util.ts Outdated Show resolved Hide resolved
@JoostK JoostK requested review from crisbeto and removed request for alxhub Jul 4, 2021
@pullapprove pullapprove bot requested a review from alxhub Jul 4, 2021
@dario-piotrowicz dario-piotrowicz requested a review from JoostK Jul 5, 2021
@dario-piotrowicz dario-piotrowicz force-pushed the dario-piotrowicz:animations-normalize-toStyles branch 2 times, most recently from 78d47c3 to b9ffddc Jul 10, 2021
@dario-piotrowicz dario-piotrowicz force-pushed the dario-piotrowicz:animations-normalize-toStyles branch from 0e99433 to cc520c0 Jul 17, 2021
@dario-piotrowicz
Copy link
Contributor Author

@dario-piotrowicz dario-piotrowicz commented Jul 17, 2021

@dario-piotrowicz You seem to have amended the last commit, which was the refactor to remove the imports. I think the goal was to create a fixup commit and squash that into the second commit.

lol you noticed that immediately! 🙈

force of habit to amend the last commit... sorry 😅 , should be all good now

@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Jul 19, 2021

FYI tests in Google's codebase (including a global run) went well.

// cc @JoostK

@AndrewKushnir
AndrewKushnir approved these changes Jul 19, 2021
View changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

@dario-piotrowicz thanks for the fix 👍
The changes look good, just left 1 minor comment on seemingly unused import.
packages/animations/browser/test/dsl/animation_trigger_spec.ts Outdated Show resolved Hide resolved
@dario-piotrowicz
fix(animations): normalize final styles in buildStyles
2686a43 
the final styles created in buildStyles lack normalization, meaning that pixel values remain as numbers (without "px") and so such properties fail to be correctly set/applied

Example: "width: 300" is applies as "width": "300" (and thus ignored) instead of the correct "width": "300px"
@dario-piotrowicz
refactor(animations): remove publicApi annotations from triggers
1eaf9ae 
the buildTrigger function and AnimationTrigger class are annotated
as publicApi which seems wrong and makes changes to the two
problematic, so they are being removed here

see: https://github.com/angular/angular/pull/42763/files#r671481902
@dario-piotrowicz
refactor(animations): remove unused animation trigger imports
f7e517a 
in the animation_trigger.ts file there are unused imports
remove them to clean up the file
@dario-piotrowicz dario-piotrowicz force-pushed the dario-piotrowicz:animations-normalize-toStyles branch from cc520c0 to f7e517a Jul 19, 2021
@AndrewKushnir AndrewKushnir removed request for IgorMinar and alxhub Jul 19, 2021
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Jul 19, 2021

@JoostK @crisbeto could you please have a final look and if everything is ok, we can probably add the PR to the merge queue?

@JoostK
Copy link
Member

@JoostK JoostK commented Jul 19, 2021

Let's do it! I was waiting for CI to go green.

alxhub added a commit that referenced this pull request Jul 20, 2021
@dario-piotrowicz @alxhub
refactor(animations): fix typo in transition animations spec file (#4…
0dfb4e8 
…2763)

fix the typo "mean't" in the transition_animation_engine spec file
to "meant" (the typo is simply in a code comment so the change
does not have any impact)

PR Close #42763
alxhub added a commit that referenced this pull request Jul 20, 2021
@dario-piotrowicz @alxhub
fix(animations): normalize final styles in buildStyles (#42763)
3cddc3d 
the final styles created in buildStyles lack normalization, meaning that pixel values remain as numbers (without "px") and so such properties fail to be correctly set/applied

Example: "width: 300" is applies as "width": "300" (and thus ignored) instead of the correct "width": "300px"

PR Close #42763
alxhub added a commit that referenced this pull request Jul 20, 2021
@dario-piotrowicz @alxhub
refactor(animations): remove publicApi annotations from triggers (#42763
6de37b8 
)

the buildTrigger function and AnimationTrigger class are annotated
as publicApi which seems wrong and makes changes to the two
problematic, so they are being removed here

see: https://github.com/angular/angular/pull/42763/files#r671481902

PR Close #42763
alxhub added a commit that referenced this pull request Jul 20, 2021
@dario-piotrowicz @alxhub
refactor(animations): remove unused animation trigger imports (#42763)
8f109da 
in the animation_trigger.ts file there are unused imports
remove them to clean up the file

PR Close #42763
@alxhub alxhub closed this in 5d7359e Jul 20, 2021
alxhub added a commit that referenced this pull request Jul 20, 2021
@dario-piotrowicz @alxhub
fix(animations): normalize final styles in buildStyles (#42763)
f12c533 
the final styles created in buildStyles lack normalization, meaning that pixel values remain as numbers (without "px") and so such properties fail to be correctly set/applied

Example: "width: 300" is applies as "width": "300" (and thus ignored) instead of the correct "width": "300px"

PR Close #42763
alxhub added a commit that referenced this pull request Jul 20, 2021
@dario-piotrowicz @alxhub
refactor(animations): remove publicApi annotations from triggers (#42763
b41a2b3 
)

the buildTrigger function and AnimationTrigger class are annotated
as publicApi which seems wrong and makes changes to the two
problematic, so they are being removed here

see: https://github.com/angular/angular/pull/42763/files#r671481902

PR Close #42763
alxhub added a commit that referenced this pull request Jul 20, 2021
@dario-piotrowicz @alxhub
refactor(animations): remove unused animation trigger imports (#42763)
0ce8f6e 
in the animation_trigger.ts file there are unused imports
remove them to clean up the file

PR Close #42763
@JoostK JoostK mentioned this pull request Jul 20, 2021
Closed
@dario-piotrowicz dario-piotrowicz mentioned this pull request Aug 14, 2021
Closed
3 of 14 tasks
@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Aug 20, 2021

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

@JoostK JoostK

@crisbeto crisbeto

@AndrewKushnir AndrewKushnir

Assignees
No one assigned
Labels
action: merge cla: yes comp: animations target: patch
Projects
None yet
Milestone
Backlog
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@dario-piotrowicz @JoostK @AndrewKushnir @crisbeto