Transform gradients in strokes#143
Conversation
| const svg = SvgElement.set(windowRef.document.createElementNS(SvgElement.svg, 'svg')); | ||
| const path = SvgElement.set(windowRef.document.createElementNS(SvgElement.svg, 'path')); |
There was a problem hiding this comment.
| const svg = SvgElement.set(windowRef.document.createElementNS(SvgElement.svg, 'svg')); | |
| const path = SvgElement.set(windowRef.document.createElementNS(SvgElement.svg, 'path')); | |
| const svg = SvgElement.set(doc.createElementNS(SvgElement.svg, 'svg')); | |
| const path = SvgElement.set(doc.createElementNS(SvgElement.svg, 'path')); |
Should I include this code style enhancement as part of this PR?
| fill = element.attributes.fill.value; | ||
| if (element.attributes) { | ||
| if (element.attributes.fill) fill = element.attributes.fill.value; | ||
| if (element.attributes.stroke) stroke = element.attributes.stroke.value; |
There was a problem hiding this comment.
Why are we checking if element.attributes exists here but not checking it in the previous if-block (if (element.attributes['stroke-width']))?
There was a problem hiding this comment.
It looks like element.attributes should exist if element.tagName exists, which is what we check for in _isContainerElement, except on Internet Explorer. So the if above would break on IE (but we don't support IE anyway)
adroitwhiz
force-pushed
the
gradient-stroke
branch
from
e6658ed to
bea1ac6
Compare
| const newStrokeRef = _createGradient(strokeGradientId, svgTag, bbox, matrix); | ||
| if (newStrokeRef) stroke = newStrokeRef; | ||
| } | ||
| } |
There was a problem hiding this comment.
It seems like fillGradientId and strokeGradientId might be the same, and if they're the same, we should try not to make a duplicate definition (although I'm not sure if it hurts anything if we do have duplicates)
Instead of checking if an element is a graphics element, check if it's "paintable" (fill/stroke applies to it). This includes several text-related elements, and excludes `use` and `image` elements.
fsih
left a comment
fsih
left a comment
There was a problem hiding this comment.
Looks good! Let's merge this after the deploy.
2 participants
Resolves
Resolves #142
Proposed Changes
This PR extends the gradient-transforming logic in
transformStrokeWidthsto transform gradients not only in fills (as it previously did) but in strokes as well.Reason for Changes
Strokes can also have gradients (and scratchfoundation/scratch-paint#1004 makes this much more likely). Previously, they weren't being taken into account when transforms were being applied.
Test Coverage
Tested manually