-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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: colorPropToSVG comply with svg specification #9408
Conversation
I am guessing this is a regression, how didn't our tests catch this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and indeed that is what you reported in your issue |
I suggest refactoring the util |
On the old code I only found a file using it, do you want to add the default value ':' to the third parameter? |
Find all usages and add what is needed IMO, very little usage. |
How to do regression testing? The old test component should not test whether the exported svg is correct. |
I m actually trying to figure out which part was broken and then i can help you writing a test. |
src/util/misc/svgParsing.ts
Outdated
|
||
if (opacity !== 1) { | ||
//change the color in rgb + opacity | ||
str += `${prop}-opacity: ${opacity.toString()}; `; | ||
str += separator === ':' ? `${prop}-opacity: ${opacity.toString()}; ` : `${prop}-opacity="${opacity.toString()}" `; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code is fine as it is, and i not holding an approval on this.
But you you want to step up the game of cleaning up, you can remove at least 2 ternaries with something like this:
let colorValue;
let opacityValue;
if (!value) {
colorValue = 'none';
} else if(...) {
colorValue = `#SVGID_${value.id}`;
} else {
....
do color value and opacity value;
}
if (separator === ':') {
return `${prop}: ${colorValue} ${opacityValue ? `${prop}-opacity: ${opacity.toString()}; ` : ''}`
} else {
return `${prop}="${color.toRgb()}" ${opacityValue ? `${prop}-opacity="${opacity.toString()}" ` : ''}`
}
this maybe is easier to read? just a suggestion follow your gut feeling is not a big deal
for the test, create a file src/shapes/Text/TextSVGExportMixin.spec.ts and inside test an svg export with background color: describe('TextSvgExport', () => {
it('exports text background color correctly', () => {
const myText = new Text('This is a test', { backgroundColor: 'rgba(100, 0, 100, 0.5)' });
const svgString = myText.toSVG();
expect(svgString).toInclude('fill=....') // find the correct string
expect(svgString).not.toInclude('fill: ....') // find the other string
})
}); in case .toInclude is not an expect matcher, just use expect(svgString.includes(...string)).toBe(true) and toBe(false) Repeat the same test with different color to test 'none' and the other cases with a gradient i suppose. To run the test, run |
i also argue if seaparator is a good variable name. Not an holding issue for me, just thought it could be pointed out |
I completely agree with you, I will improve it and add a new test file to complete the testing of the function, thanks for you help |
Work done, everything looks fine |
We also need a new line in the changelog.md file in the root folder, sorry i forgot to ask you that |
Already added, please try again |
Since @ShaMan123 requested some changes, let's give him time to review, but i think this is great. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job guys
Motivation
@closes #9407
Description
Change the output content of the colorPropToSVG function
Changes
Change ':' to '='
How to write tests correctly, I need a little help, old test component seem doesn't work. This change is very small. Can anyone help me add test? I'm a little busy recently, Sorry