salesforce / lwc

⚡️ LWC - A Blazing Fast, Enterprise-Grade Web Components Foundation
https://lwc.dev
Other
1.59k stars 386 forks source link

fix: inline style parsing for "! important" #4329

Closed Bradels closed 1 day ago

Bradels commented 1 week ago

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

If DISABLE_STATIC_CONTENT_OPTIMIZATION=1 "! important" will now be parsed correctly. This approach uses Regex to find the !important tag at the end of the string, it is generous in that it allows for abitrary amounts of white space. Happy to update to only look for the one scenario outlined in the original issue.

GUS work item

salesforce-cla[bot] commented 1 week ago

Thanks for the contribution! Before we can merge this, we need @Bradels to sign the Salesforce Inc. Contributor License Agreement.

nolanlawson commented 5 days ago

Hi @Bradels thanks for the PR! This looks good to me overall, but would you mind adding a test to confirm that we parse ! important correctly? Here is an example of how we currently test this kind of feature:

https://github.com/salesforce/lwc/blob/05743434d7d24e8d068c3487df7c40d4376cb281/packages/%40lwc/integration-karma/test/rendering/programmatic-stylesheets/index.spec.js#L25-L27

In this case, we would need a component containing the ! important style and then to confirm that ! important is overriding some other CSS property correctly.

Bradels commented 5 days ago

Hi @nolanlawson Thank you for the reply. I will make those tests and update this PR. CLA still in progress, waiting on reponse from employer.

Bradels commented 5 days ago

Hi @nolanlawson , tests are added to confirm CSS important declaration is being parsed correctly and that it is overriding an existing CSS property that otherwise would be applied.

This is based on the original tests you created in https://github.com/salesforce/lwc/commit/81365992e05585daadc4b17a9184ab7bfdb9b54a for #4125

Worth noting the getComputedStyle(div).getPropertyPriority('color') will return a blank string, even if the original rule was "important". For this reason I used the commits approach of div.style.getPropertyPriority('color') to verify the important declaration is being correctly parsed.

P.S. I may have updated my PR in an incorrect way, as it includes a commit that is not mine. If needed I can attempt to fix this by recreating my PR.

Bradels commented 4 days ago

Hey @nolanlawson, thank you for taking the time to look into my PR. Really grateful. I have updated the test cases to reflect case insensitive scenarios. Also added a check to make sure the test is actually finding some elements and a small refactor of the regex to match the style of the other functions.

Bradels commented 2 days ago

CLA signed

nolanlawson commented 1 day ago

/nucleus test