fix(theming): fix CSS with nested rules parsing when registering a theme #9872

natete · 2016-10-20T12:21:26Z

Replace the regex used to split the CSS string with a function that takes into account nested rules.

googlebot · 2016-10-20T12:21:29Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

natete · 2016-10-20T12:24:33Z

I signed it!

googlebot · 2016-10-20T12:24:35Z

CLAs look good, thanks!

crisbeto

Haven't looked into the functionality, but I left a few minor notes on the code formatting.

src/core/services/theming/theming.js

src/core/services/theming/theming.spec.js

src/core/services/theming/theming.js

src/core/services/theming/theming.spec.js

crisbeto · 2016-10-22T04:39:57Z

LGTM from me regarding the coding style changes.

ThomasBurleson · 2016-10-24T10:42:41Z

@natete - please squash your commits. Thank you.

src/core/services/theming/theming.js

src/core/services/theming/theming.spec.js

clshortfuse

I've just like to see some minor syntax changes as I noted in the review. Other than that, LGTM!

Thanks a lot @natete for not only finding this issue, but providing a fix!

src/core/services/theming/theming.js

src/core/services/theming/theming.spec.js

Replace the regex used to split the CSS string with a function that takes into account nested rules. Fixes angular#9869

natete · 2016-10-25T18:17:23Z

Thank you for helping me, I've learned a lot :)
And thank you for your work! 😃

ThomasBurleson · 2016-10-25T18:27:55Z

@clshortfuse - #ftw!
@natete - thank you for your PR. If you would like to work on more issues and features. Let us know. ;-)

natete · 2016-10-25T20:59:20Z

@ThomasBurleson - for sure, I'd love to help whenever you need 👍

Splaktar · 2019-01-22T01:42:12Z

@natete I just wanted to check with you to see if you still have any interest in this PR. The issue does still appear to be a problem, but in order to get this in, we would need to work together with the caretakers to identify and eliminate any changes that could be breaking. If you don't have time or interest in working on this more, please let me know and I will open a new PR to continue the work. Thank you very much for your contribution and I'm sorry that it has gotten stuck like this for so long.

natete · 2019-01-22T08:15:18Z

Hi, I haven't use AngularJS for the last two years. Anyway, let me know how can I help you and I'll try my best.

Splaktar

This looks really good. Thank you for this hard work and for your patience.

I've rebased this and will try to get it merged in PR #12049.

natete · 2020-12-07T22:02:32Z

@googlebot I consent

ThomasBurleson added this to the 1.1.3 milestone Oct 20, 2016

ThomasBurleson assigned clshortfuse Oct 20, 2016

ThomasBurleson added P5: nice to have These issues will not be fixed without community contributions. in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Oct 20, 2016

ThomasBurleson assigned EladBezalel and clshortfuse and unassigned clshortfuse Oct 20, 2016

crisbeto suggested changes Oct 21, 2016

View reviewed changes

src/core/services/theming/theming.js Show resolved Hide resolved

src/core/services/theming/theming.js Show resolved Hide resolved

src/core/services/theming/theming.spec.js Show resolved Hide resolved

src/core/services/theming/theming.spec.js Show resolved Hide resolved

clshortfuse suggested changes Oct 21, 2016

View reviewed changes

src/core/services/theming/theming.js Show resolved Hide resolved

src/core/services/theming/theming.js Show resolved Hide resolved

clshortfuse reviewed Oct 21, 2016

View reviewed changes

src/core/services/theming/theming.spec.js Show resolved Hide resolved

crisbeto approved these changes Oct 22, 2016

View reviewed changes

clshortfuse suggested changes Oct 24, 2016

View reviewed changes

src/core/services/theming/theming.js Show resolved Hide resolved

src/core/services/theming/theming.spec.js Show resolved Hide resolved

clshortfuse approved these changes Oct 24, 2016

View reviewed changes

src/core/services/theming/theming.js Show resolved Hide resolved

src/core/services/theming/theming.spec.js Show resolved Hide resolved

src/core/services/theming/theming.spec.js Show resolved Hide resolved

clshortfuse added P0: critical Critical issues that must be addressed immediately. and removed P5: nice to have These issues will not be fixed without community contributions. labels Oct 24, 2016

natete force-pushed the fix/css-parsing branch from 140ecee to 0168874 Compare October 25, 2016 07:11

fix(theming): fix CSS with nested rules parsing when registering a theme

9f0072c

Replace the regex used to split the CSS string with a function that takes into account nested rules. Fixes angular#9869

natete force-pushed the fix/css-parsing branch from 0168874 to 9f0072c Compare October 25, 2016 07:23

ThomasBurleson added needs: presubmit and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Oct 25, 2016

ThomasBurleson modified the milestones: 1.1.3, 1.1.2 Oct 25, 2016

Splaktar modified the milestones: 1.1.12, 1.1.13 Jan 3, 2019

googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Jan 22, 2019

Splaktar modified the milestones: 1.1.13, 1.1.14 Feb 10, 2019

Splaktar modified the milestones: 1.1.14, 1.1.15 Mar 14, 2019

Splaktar modified the milestones: 1.1.15, 1.1.16, 1.1.18, 1.1.19 Mar 29, 2019

Splaktar modified the milestones: 1.1.19, 1.1.21 May 30, 2019

Splaktar modified the milestones: 1.1.21, 1.1.22 Aug 15, 2019

Splaktar modified the milestones: 1.1.22, 1.1.23 Oct 22, 2019

Splaktar modified the milestones: 1.1.23, 1.2.0 May 21, 2020

Splaktar modified the milestones: 1.2.0, - Backlog Jul 28, 2020

Splaktar removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs - Breaking Change labels Dec 6, 2020

Splaktar removed this from the - Backlog milestone Dec 6, 2020

Splaktar mentioned this pull request Dec 6, 2020

fix(theming): fix CSS with nested rules parsing when registering a theme #12049

Merged

3 tasks

Splaktar reviewed Dec 6, 2020

View reviewed changes

Splaktar closed this in #12049 Dec 9, 2020

fix(theming): fix CSS with nested rules parsing when registering a theme #9872

fix(theming): fix CSS with nested rules parsing when registering a theme #9872

Uh oh!

Conversation

natete commented Oct 20, 2016

Uh oh!

googlebot commented Oct 20, 2016

Uh oh!

natete commented Oct 20, 2016

Uh oh!

googlebot commented Oct 20, 2016

Uh oh!

crisbeto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

crisbeto commented Oct 22, 2016

Uh oh!

ThomasBurleson commented Oct 24, 2016

Uh oh!

Uh oh!

Uh oh!

clshortfuse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

natete commented Oct 25, 2016

Uh oh!

ThomasBurleson commented Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natete commented Oct 25, 2016

Uh oh!

Splaktar commented Jan 22, 2019

Uh oh!

natete commented Jan 22, 2019

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

natete commented Dec 7, 2020

Uh oh!

Uh oh!

crisbeto left a comment •

edited

Loading

ThomasBurleson commented Oct 25, 2016 •

edited

Loading