-
Notifications
You must be signed in to change notification settings - Fork 30
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
Support rendering of ghost element when creating elements #301
Conversation
- Introduce new element template module -- Adding new elements to the diagram based on templates -- Removing templates again - Support local bounds calculation for client-only added elements -- Must trigger RequestBounds for hidden view calculation -- Must not modify main view until ComputedBounds response is handled -- Mark ResizeHandles as not to be considered for bounds calculation -- Fix positioning of decorations for hidden view calculation - Add ghost element extension to tool palette -- Optional ghost element field in the trigger node creation action -- Ghost element is added as feedback to the mouse cursor -- If ghost element is dynamic, templates (palette items) are reloaded - Bonus: Re-calculate local bounds for resize behavior Fixes eclipse-glsp/glsp#1159
86c4644
to
f3444a3
Compare
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.
In general the change works really well!
I have a couple of inline comments and suggestions but other than that everything looks good to me.
Could you please make sure that all of you newly introduced functions,classes etc. are added to the main index and that add (high level) jsdoc to them?
While marking the local-only RequestBounds/ComputedBounds
as server actions works it is not really intuitive and rather confusing. I'm wondering whether we should add the a more generic approach here to explicitly restrict a specific action that is normally handle on both client and server to one target.
@@ -35,3 +35,13 @@ export function isArgsAware(element: GModelElement): element is GModelElement & | |||
export function hasArgs(element?: GModelElement): element is GModelElement & Required<ArgsAware> { | |||
return element !== undefined && isArgsAware(element) && element.args !== undefined; | |||
} | |||
|
|||
export function ensureArgs(element?: GModelElement): element is GModelElement & Required<ArgsAware> { | |||
if (element === undefined || !isArgsAware(element)) { |
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.
Minor: For reuse-ability it would be nicer if we move the undefined check into the isArgsAware
function and make the element arg optional
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 idea!
packages/client/src/features/bounds/glsp-hidden-bounds-updater.ts
Outdated
Show resolved
Hide resolved
packages/client/src/features/bounds/glsp-hidden-bounds-updater.ts
Outdated
Show resolved
Hide resolved
packages/client/src/features/bounds/set-bounds-feedback-command.ts
Outdated
Show resolved
Hide resolved
Bonus: Simplify hidden bounds customization via CSS
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.
Thanks! Everything works great now.
There is only one minor issue with the "stamp-mode". The ghost element template is not updated inbetween node creation. It still represents the initial label of the first creation
Peek.2023-11-13.14-07.mp4
@tortmayr I agree that this would be a nice feature to have. However, currently there is no way to simply reload trigger actions and then to uniquely identify them so we can easily replace the action and template that we are currently using. So I pulled this out into an enhancement issue (eclipse-glsp/glsp#1161) as it would require more work that I want to consider out of scope for this ticket. What do you think? |
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.
LGTM! Thanks!
…sp#301) * Support rendering of ghost element when creating elements - Introduce new element template module -- Adding new elements to the diagram based on templates -- Removing templates again - Support local bounds calculation for client-only added elements -- Must trigger RequestBounds for hidden view calculation -- Must not modify main view until ComputedBounds response is handled -- Mark ResizeHandles as not to be considered for bounds calculation -- Fix positioning of decorations for hidden view calculation - Add ghost element extension to tool palette -- Optional ghost element field in the trigger node creation action -- Ghost element is added as feedback to the mouse cursor -- If ghost element is dynamic, templates (palette items) are reloaded - Bonus: Re-calculate local bounds for resize behavior Fixes eclipse-glsp/glsp#1159
…sp#301) * Support rendering of ghost element when creating elements - Introduce new element template module -- Adding new elements to the diagram based on templates -- Removing templates again - Support local bounds calculation for client-only added elements -- Must trigger RequestBounds for hidden view calculation -- Must not modify main view until ComputedBounds response is handled -- Mark ResizeHandles as not to be considered for bounds calculation -- Fix positioning of decorations for hidden view calculation - Add ghost element extension to tool palette -- Optional ghost element field in the trigger node creation action -- Ghost element is added as feedback to the mouse cursor -- If ghost element is dynamic, templates (palette items) are reloaded - Bonus: Re-calculate local bounds for resize behavior Fixes eclipse-glsp/glsp#1159
Introduce new element template module
-- Adding new elements to the diagram based on templates
-- Removing templates again
Support local bounds calculation for client-only added elements
-- Must trigger RequestBounds for hidden view calculation
-- Must not modify main view until ComputedBounds response is handled
-- Mark ResizeHandles as not to be considered for bounds calculation
-- Fix positioning of decorations for hidden view calculation
Add ghost element extension to tool palette
-- Optional ghost element field in the trigger node creation action
-- Ghost element is added as feedback to the mouse cursor
-- If ghost element is dynamic, templates (palette items) are reloaded
Bonus: Re-calculate local bounds for resize behavior
Fixes eclipse-glsp/glsp#1159
(Mouse cursor is also shown in the application but not on the video unfortunately)
Can be tested with eclipse-glsp/glsp-server-node#65