feat(svgcanvas): Trigger 'changed' event on text attribute modifications#1085
Merged
Conversation
This update adds a call to the 'changed' event for various text attribute methods, ensuring that changes to text elements are properly tracked and reflected in the SVG canvas. The methods updated include setBold, setItalic, setTextAnchor, setLetterSpacing, setWordSpacing, setTextLength, setLengthAdjust, setFontFamily, setFontColor, and setFontSize.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR updates SVG text attribute setters in svgcanvas so that any change to text styling or layout consistently fires the global 'changed' event, allowing downstream listeners (e.g., history/undo, UI sync) to track text modifications as they happen. Sequence diagram for SVG text attribute update triggering changed eventsequenceDiagram
actor User
participant TextTool
participant TextAttributeSetters
participant SvgCanvas
participant HistoryManager
participant UiSync
User->>TextTool: change font size
TextTool->>TextAttributeSetters: setFontSizeMethod(val)
TextAttributeSetters->>SvgCanvas: getSelectedElements()
SvgCanvas-->>TextAttributeSetters: selectedElements
TextAttributeSetters->>SvgCanvas: changeSelectedAttribute(font-size, val, selectedElements)
alt firstCharacterBeingTyped
TextAttributeSetters->>SvgCanvas: textActions.setCursor()
end
TextAttributeSetters->>SvgCanvas: call(changed, selectedElements)
SvgCanvas->>HistoryManager: onChanged(selectedElements)
SvgCanvas->>UiSync: onChanged(selectedElements)
Sequence diagram for SVG text color update triggering changed eventsequenceDiagram
actor User
participant ColorPicker
participant TextAttributeSetters
participant SvgCanvas
participant HistoryManager
participant UiSync
User->>ColorPicker: pick text color
ColorPicker->>TextAttributeSetters: setFontColorMethod(val)
TextAttributeSetters->>SvgCanvas: getSelectedElements()
SvgCanvas-->>TextAttributeSetters: selectedElements
TextAttributeSetters->>SvgCanvas: setCurText(fill, val)
TextAttributeSetters->>SvgCanvas: changeSelectedAttribute(fill, val)
TextAttributeSetters->>SvgCanvas: call(changed, selectedElements)
SvgCanvas->>HistoryManager: onChanged(selectedElements)
SvgCanvas->>UiSync: onChanged(selectedElements)
Class diagram for updated svgcanvas text attribute setter methodsclassDiagram
class TextAttributeSetters {
+setBoldMethod(b)
+setItalicMethod(i)
+setTextAnchorMethod(value)
+setLetterSpacingMethod(value)
+setWordSpacingMethod(value)
+setTextLengthMethod(value)
+setLengthAdjustMethod(value)
+setFontFamilyMethod(val)
+setFontColorMethod(val)
+setFontSizeMethod(val)
}
class SvgCanvas {
+getSelectedElements()
+changeSelectedAttribute(name, value, elements)
+setCurText(name, value)
+call(eventName, elements)
+textActions
}
class TextActions {
+setCursor()
}
class HistoryManager {
+onChanged(elements)
}
class UiSync {
+onChanged(elements)
}
SvgCanvas *-- TextActions
TextAttributeSetters ..> SvgCanvas : uses
SvgCanvas ..> HistoryManager : emits_changed
SvgCanvas ..> UiSync : emits_changed
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new
svgCanvas.call('changed', selectedElements)calls are repeated with the same pattern across many setters; consider extracting a small helper (e.g.,notifyTextChange(selectedElements)) to reduce duplication and keep future behavior changes centralized. - There is an inconsistency in what gets passed to
changed: several methods operate ontextElementsbut callchangedwithselectedElements(andsetFontColorMethodnow fetchesselectedElementsjust for this); consider standardizing whether the event should receive all selected elements or only the text elements actually modified. - You might want to guard the
svgCanvas.call('changed', ...)calls so they only fire when there is at least one relevant text element or when the attribute value actually changed, to avoid emitting redundant change events on no-op calls.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `svgCanvas.call('changed', selectedElements)` calls are repeated with the same pattern across many setters; consider extracting a small helper (e.g., `notifyTextChange(selectedElements)`) to reduce duplication and keep future behavior changes centralized.
- There is an inconsistency in what gets passed to `changed`: several methods operate on `textElements` but call `changed` with `selectedElements` (and `setFontColorMethod` now fetches `selectedElements` just for this); consider standardizing whether the event should receive all selected elements or only the text elements actually modified.
- You might want to guard the `svgCanvas.call('changed', ...)` calls so they only fire when there is at least one relevant text element or when the attribute value actually changed, to avoid emitting redundant change events on no-op calls.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…anged' event emission This update introduces helper functions to streamline the handling of selected text elements and their attribute changes. The methods setBold, setItalic, setTextAnchor, setLetterSpacing, setWordSpacing, and setTextLength now utilize these helpers to filter and notify only modified text elements, improving performance and clarity in event emissions.
jfhenon
approved these changes
Mar 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This update adds a call to the 'changed' event for various text attribute methods, ensuring that changes to text elements are properly tracked and reflected in the SVG canvas. The methods updated include setBold, setItalic, setTextAnchor, setLetterSpacing, setWordSpacing, setTextLength, setLengthAdjust, setFontFamily, setFontColor, and setFontSize.
PR description
Checklist
Note that we require UI tests to ensure that the added feature will not be
nixed by some future fix and that there is at least some test-as-documentation
to indicate how the fix or enhancement is expected to behave.
npm test, ensuring linting passes and that Cypress UI tests keepcoverage to at least the same percent (reflected in the coverage badge
that should be updated after the tests run)
help both for future users and for the PR reviewer.
Summary by Sourcery
Ensure text formatting updates on SVG text elements trigger canvas change notifications.
Bug Fixes:
Enhancements: