Fix regex in resolve-value.js to allow nested CSS functions

juliovedovatto commented 5 years ago

This fix will make work, when using nested CSS functions in the same declaration.

See https://github.com/MadLittleMods/postcss-css-variables/issues/96

juliovedovatto commented 5 years ago

First, thank you @MadLittleMods for the opportunity to let me help with this small fix 😄

My Scenario:

Previously, I just did a hotfix, without testing it properly, because deadline was short. Due IE11 browser, we had to convert every var() to its own original value. We used https://material.io grid layout and it contains A LOT of CSS variables.

My new merge proposal:

I decided to change my approach, and improved the RegExp sentence. I added an extra instruction to help match nested functions with var() declarations. With a fallback using the original regex block.

In addition, I had to change your approach on how replace results. I added a while block (not sure if is a good idea), to replace until no var() is found.

To finish, I just updated nested-inside-other-func test, with two situations: one with a nested function with a declared var() and another with a undefined var(). Using the orginal RegExp it fails, using my proposal, it passes 🎉

Please check if my logic makes sense to you and if you can imagine any extra scenario to test.

Cheers from Brazil 🇧🇷

juliovedovatto commented 5 years ago

@MadLittleMods I made more code changes, just to adapt your logic in some special scenarios.

I was facing issues with RegExp to match few nested inside css functions.

I spent a good time trying to create a good regex to help, but due the fact JS RegExp engine is limited, I had to change my approach.

The match-recursive npm lib was added, to help match several var() declaration in the same line. Additionally, I changed a few chunks of the code to fit my logic into your code. Also, I changed the original regex in RE_VAR_FUNC variable, just to help fill variablesUsedInValue array.

I added more related tests and worked ok.

Could you take a look and make some tests? I believe this is a good candidate to merge 🤓

Cheers!

juliovedovatto commented 5 years ago

@MadLittleMods I did code changes again, please review. I think we are in the right track :)

Now, it will use a while loop and balanced-match npm lib to match var() declarations and resolve them. This should cover nested CSS functions.

In addition, I added one more test for css nested functions. It should be all covered now.

Cheers!

juliovedovatto commented 5 years ago

@MadLittleMods Another code review round done. Please check.

juliovedovatto commented 5 years ago

@MadLittleMods any updates?

colinhowells commented 5 years ago

Thanks everyone for your hard work on this – I've been watching this bit of the HiQ framework

--radio-border-width: var(--hiq-radio-border-width, 1px) solid var(--hiq-radio-border-color, transparent);
--radio-border-color: var(--hiq-radio-border-color, transparent);
border: var(--radio-border-width) solid var(--radio-border-color);

get compiled to:

border: 1px) solid var(--hiq-radio-border-color, transparent solid transparent;

and had no clue why. If it was defeated by regex, that'd make sense ...

juliovedovatto commented 5 years ago

@MadLittleMods Done with the requested changes. Could you take a look? :)

@colinhowells You are right, it was defeated by regex. The intend of my proposal is to fix when some complex var() usage, specially when nested declarations appear.

juliovedovatto commented 5 years ago

@MadLittleMods I did some of your requests, could you review again?

Also, check my comment https://github.com/MadLittleMods/postcss-css-variables/pull/97#discussion-diff-296426364R32

juliovedovatto commented 4 years ago

@MadLittleMods any news?

juliovedovatto commented 4 years ago

@MadLittleMods sorry to bump this PR, but could you take a look in the latest changes sent?