-
Notifications
You must be signed in to change notification settings - Fork 3.8k
fix!: remove MarkerSvg and uses #8991
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!: remove MarkerSvg and uses #8991
Conversation
|
Does not fix #8968 |
bd2ff97 to
d9548a0
Compare
cpcallen
left a comment
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.
Opinion: I would (and now have) described issue #8962 as a task rather than a bug, so I'd have titled this PR and its main commits as chore: rather than fix:.
| renderManagement | ||
| .finishQueuedRenders() | ||
| .then(() => void this.markerManager.updateMarkers()); |
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.
Should render no longer call renderManagement.finishQueuedRenders() at all?
(Maybe the queued renderers will be finished automatically, and the method call is not needed if you don't need the resulting promise?)
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.
Yes, queued renders will finish automatically, and the method call is specifically to be able to get a promise and run after the renders have finished.
| export interface IASTNodeLocationSvg extends IASTNodeLocation { | ||
|
|
||
| } | ||
| export interface IASTNodeLocationSvg extends IASTNodeLocation {} |
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.
Could this interface not be deleted entirely, now that it is empty?
I suppose we could formally deprecate it first, but since anyone who was calling setFooSvg on an IASTNodeLocationSvg instance already has broken code I'm not sure it's worth it.
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.
Filed as #9006
|
For some reason GitHub hasn't run build, format or lint workflows, and I can't figure out how to kick it in to action. |
38ad3ea to
d9548a0
Compare
d9548a0 to
c8ec2f2
Compare
|
Workflows ran and passed. |
bb76d6e
into
RaspberryPiFoundation:rc/v12.0.0
The basics
The details
Resolves
Fix for #8962
Proposed Changes
Commit 1: Delete the actual drawer, subclasses, and uses
Commit 2: Delete residual SVG elements that were rendered through the cursor/marker flow instead of through the normal block flow.
Commits 3 and 4: lint and format.
Reason for Changes
MarkerSvgand friends represented a way to draw the cursor that was outside of the standard rendering flow. These SVG elements were children of the standard block (field, workspace, etc) SVG elements, but were updated through explicit calls tosetCurNodeon the cursor.Ben's focus changes updated focus rendering to use elements that are rendered/updated through the standard rendering flow. Where possible, focus is shown using CSS rather than by creating new elements.
Test Coverage
Documentation
Additional Information
hide().IASTNodeLocationSvgcan be removed as a follow-up, if it hasn't already been removed during refactors ofASTNode.