-
Notifications
You must be signed in to change notification settings - Fork 229
chore(compass-crud): adjust cancel editing logic COMPASS-9564 #7107
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
Conversation
doc.on(ElementEvents.Reverted, onUpdate); | ||
doc.on(ElementEvents.Invalid, onElementInvalid); | ||
doc.on(ElementEvents.Valid, onElementValid); | ||
doc.on(DocumentEvents.UpdateStarted, onUpdateStart); |
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.
I believe 'update-start'
.. etc. can be encompassed under DocumentEvents
? Not sure if there was an explicit reason they weren't before.
I also renamed their constant string values for consistency and I wouldn't expect this would break things unless we have some external dependentes that expect these exact strings which doesn't seem like 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.
Pull Request Overview
This PR centralizes document and element event constants into DocumentEvents
and ElementEvents
, updates all event references to use those constants, and adjusts the cancel-editing UI logic.
- Introduce
DocumentEvents
andElementEvents
constants and their types. - Replace raw event strings and default exports with the new constants across the document and editor modules.
- Rename the context-menu action from “Stop editing” to “Cancel editing”, group menu items, add handling for
EditingFinished
, and update tests.
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
packages/hadron-document/src/index.ts | Refactor exports to use centralized DocumentEvents and ElementEvents |
packages/hadron-document/src/document-events.ts | Introduce DocumentEvents constant and DocumentEventsType |
packages/hadron-document/src/element-events.ts | Introduce ElementEvents constant and ElementEventsType |
packages/hadron-document/src/document.ts | Replace hardcoded event strings with DocumentEvents constants |
packages/hadron-document/src/element.ts | Replace bubbling event references with ElementEvents constants |
packages/hadron-document/src/editor/*.ts | Updated imports and event emissions to use ElementEvents |
packages/compass-crud/src/stores/crud-store.spec.ts | Update tests to use DocumentEvents constants and onceDocumentEvent helper |
packages/compass-crud/src/components/use-document-item-context-menu.tsx | Rename “Stop editing” to “Cancel editing” and group context menu items |
packages/compass-components/src/components/document-list/document-edit-actions-footer.tsx | Add EditingFinished listener, reset status, and update cancel logic |
Comments suppressed due to low confidence (1)
packages/compass-components/src/components/document-list/document-edit-actions-footer.tsx:186
- Consider adding a unit test to verify that the status resets when the
EditingFinished
event is emitted, ensuring the new handler works as intended.
doc.on(DocumentEvents.EditingFinished, onEditingFinished);
packages/compass-components/src/components/document-list/document-edit-actions-footer.tsx
Outdated
Show resolved
Hide resolved
packages/compass-components/src/components/document-list/document-edit-actions-footer.tsx
Outdated
Show resolved
Hide resolved
@@ -110,6 +115,15 @@ function waitForState(store, cb, timeout?: number) { | |||
return waitForStates(store, [cb], timeout); | |||
} | |||
|
|||
function onceDocumentEvent( |
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.
[nitpick] Instead of casting to Node’s EventEmitter and using once
, leverage the EventEmitter3 API directly, e.g.: return new Promise(resolve => doc.once(event, (...args) => resolve(args)));
Copilot uses AI. Check for mistakes.
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.
I mean... not the worst idea I guess
62a393b
to
b3ceb4e
Compare
4ca0fb3
to
529855f
Compare
Review
It is best to review this PR in individual commits:
fe04ed8 - largely a drive-by clean-up of the current event setup by adding more of the values into the
DocumentEvents
constant and cleaning up our export structure.e7dc505 - adds a
DocumentEvents.EditingFinished
event handling todocument-edit-actions-footer.tsx
so its status can be correctly changed when editing is finished through means other than the cancel button. Also moves the action into its own group at the top and renames it.Motivation
'Stop editing' when right clicking does not fully stop editing the element. This is because the Editing Actions footer does not change status when editing is finished.
'Stop editing' should also be bumped up into a separate group and renamed to Cancel editing as a note from design.