fix: colorPropToSVG comply with svg specification

[tyutjohn]

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

[ShaMan123]

I am guessing this is a regression, how didn't our tests catch this?

[ShaMan123]

This seems wrong to me
Looking at usages in fabric the only usage that looks wrong is

[ShaMan123]

and indeed that is what you reported in your issue

[ShaMan123]

I suggest refactoring the util colorPropToSVG to accept a third arg separator: ':' | '='

[tyutjohn]

I suggest refactoring the util colorPropToSVG to accept a third arg separator: ':' | '='

On the old code I only found a file using it, do you want to add the default value ':' to the third parameter?

[ShaMan123]

Find all usages and add what is needed IMO, very little usage.

[tyutjohn]

I am guessing this is a regression, how didn't our tests catch this?

How to do regression testing? The old test component should not test whether the exported svg is correct.

[asturur]

I m actually trying to figure out which part was broken and then i can help you writing a test.

[asturur]

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

[asturur]

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 npm run test:jest Text/TextSVGExportMixin.spec.ts

[asturur]

i also argue if seaparator is a good variable name.
Is kind of indirect, because the separator is the ':' for styles and '=' for attributes, but what change is not just the separator but the quotes too.
A non expert dev could say, ok the function support a separator i can add a different one, while what the function does is just forInlineStyle true/false or forAttributes false/true. Probably a boolean is nicer than the separator string.

Not an holding issue for me, just thought it could be pointed out

[tyutjohn]

i also argue if seaparator is a good variable name. Is kind of indirect, because the separator is the ':' for styles and '=' for attributes, but what change is not just the separator but the quotes too. A non expert dev could say, ok the function support a separator i can add a different one, while what the function does is just forInlineStyle true/false or forAttributes false/true. Probably a boolean is nicer than the separator string.

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

[tyutjohn]

i also argue if seaparator is a good variable name. Is kind of indirect, because the separator is the ':' for styles and '=' for attributes, but what change is not just the separator but the quotes too. A non expert dev could say, ok the function support a separator i can add a different one, while what the function does is just forInlineStyle true/false or forAttributes false/true. Probably a boolean is nicer than the separator string.
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

[asturur]

We also need a new line in the changelog.md file in the root folder, sorry i forgot to ask you that

[tyutjohn]

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

[asturur]

Since @ShaMan123 requested some changes, let's give him time to review, but i think this is great.
Thank your for your contribution

[ShaMan123]

Good job guys