Skip to content
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: Add support for Css variables #209

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

TOKdawn
Copy link

@TOKdawn TOKdawn commented Apr 22, 2022

Hi,

I add a support for Css variables

I analyzed cssValue then capture with /var\(.*?\)/gi

Processing with function getCssValue().

Best regards

Fixes ##206

Fix: Optimized code structure

Optimized code structure

Fix: Add support for Css variables
Copy link
Member

@HackbrettXXX HackbrettXXX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Please add some test cases. I'll have a closer look at the code afterwards.

@TOKdawn
Copy link
Author

TOKdawn commented Apr 24, 2022

Thanks for the PR. Please add some test cases. I'll have a closer look at the code afterwards.

hi,
I add the css-variables into test-unit;
But i can’t adopt test;
I think it may be because of my reference.pdf version is jsPDF2.00;
I test in /index.html is correct,and can normal download;
I hope you can test it;
Best regards

Copy link
Member

@HackbrettXXX HackbrettXXX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the test case. I think the approach is a bit flawed: the lookup for a variable value includes all available CSS rule. However, it should only include the rules that apply to the current element.

Comment on lines 2 to 4
<style>.bluePolyline {stroke: var(--color); } .polyLineColors {--color: blue;}</style>
<style>.redPolyline { --colorRed: red; stroke: var(--colorRed);}</style>
<style>.greenPolyline { stroke: var(--colorGreen);}</style>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just nitpicking, but "textColors" would probably be more understandable than "polylineColors" here.

Copy link
Author

@TOKdawn TOKdawn Apr 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep , I just copy code from issues. not rename class.
I will modify them

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’re right, maybe I should rework my code withelement.style.getPropertyValue(“—my-var”);
and add support for custom property fallback values.
I will try to fix it.
Thank you for your review.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't checked the code too much,
I don't know if I can use the DOM API to get the current element style or analytic CSS-variables key,
if there is no DOM, maybe I can only get the CSS-variables key through Regular Expression.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the DOM API.

<text class="green greenPolyline" y="20" x="20">Value With Space</text>
<text y="40" x="20" class="rgb" >RGB Color</text>
<text y="60" x="20" class="space">Var With Space </text>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test case like this

<style>.variable { stroke: var(--color); }</style>
<style>.red { --color red; }</style>
<style>.green { --color green; }</style>
<text class="red variable">Red</text>
<text class="green variable">Green</text>

And a couple of tests that test the inheritance of the variables.
Also add a test case where the variable is not defined.
Add test cases for CSS specificity.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this is my first PR, not well thought out, I will add and test them

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,I fix all bug.and add many test cases.
Looking forward to your review.
QvQ

Copy link
Member

@HackbrettXXX HackbrettXXX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, I was really busy. Thanks for the changes.


<style>.TestValue { stroke: var(--colorGreen);}</style>
<style>.green {--colorGreen: #009900 }</style>
<text class="green greenPolyline" y="20" x="20">Test Value With Space</text>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

greenPolyline seems to be a leftover.

Comment on lines 152 to 158
getCssValue(selector: string): string | undefined {
const value: string = selector.replace(/^\s+|\s+$/g, '')
this.cssVariableList = []
if (this.analysisCssVariables(value, 0)) {
}
return undefined
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be unused?

Comment on lines 119 to 133
valueIndex = 0
valueName = ''
//exclude 'var(' and ')' form cssValueString
for (let ch = 0; ch < valueString.length; ch++) {
if (valueString[ch] == 'v' && valueString.slice(ch, ch + 4) == 'var(') {
valueIndex++
ch += 3
continue
}
if (valueString[ch] == ')' && valueIndex > 0) {
valueIndex--
continue
}
valueName += valueString[ch]
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do I need this loop? Couldn't we just adjust the slice call?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value maybe like this var(--colorOne,var(--colorTwo,rgb(0,0,0))).
I was thinking of removing var( and ),then split with ,
Do you have a better solution?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi!
I refactored my code,
reduced the loop level,
reduced unnecessary calculations,
Put the code in a more logical place.
Looking forward to your review.

originalCss: valueString,
valueName: valueName.split(varReg)
})
return this.analysisCssVariables(selector, untreatedIndex + varEnd)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the recursive approach. A loop would probably be more readable and better performance-wise.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK,I'll change

export class StyleSheets {
private rootSvg: Element
private readonly loadExternalSheets: boolean
private readonly styleSheets: CSSStyleSheet[]

private cssVariableList: CssVariable[]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need the field. Just return the list from analysisCssVariables (or pass a local list as 'out' variable).

const cssValue: string = mostSpecificRule.style.getPropertyValue(propertyCss)
const varReg = /var\(.*?\)/gi
if (cssValue && varReg.test(cssValue)) {
const originalValue = cssValue.replace(/^\s+|\s+$/g, '')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.trim()?

let res = originalValue
this.cssVariableList = []
if (this.analysisCssVariables(originalValue, 0)) {
this.cssVariableList.map(CssVariable => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lower case var

for (let i = 0; i < CssVariable.valueName.length; i++) {
const name = CssVariable.valueName[i].replace(/^\s+|\s+$/g, '')
if (name.slice(0, 2) == '--') {
const css = getComputedStyle(node).getPropertyValue(name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getComputedStyle won't work, because the SVG is not part of document.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a long time,Looking forward to your review.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly, @HackbrettXXX has not been available in recent months. He will get back to you, I am very sure, once he is back to work. Please allow him a bit more time.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK,I'll be waiting.

Copy link
Member

@HackbrettXXX HackbrettXXX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm back and finally have time to review this again. Sorry for the very long delay.

Please add still more test cases (like mentioned in my previous comment). Especially for CSS specificity and my example test case.

Please also try to make the code more readable, extract some functions, make the loop more readable, etc.

@HackbrettXXX HackbrettXXX marked this pull request as draft December 11, 2023 09:14
@yWorks yWorks deleted a comment from enersis-pst Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants