-
Notifications
You must be signed in to change notification settings - Fork 247
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
DocumentEventsandElementEventsconstants 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
EditingFinishedevent 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
| return waitForStates(store, [cb], timeout); | ||
| } | ||
|
|
||
| function onceDocumentEvent( |
Copilot
AI
Jul 14, 2025
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)));
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
DocumentEventsconstant and cleaning up our export structure.e7dc505 - adds a
DocumentEvents.EditingFinishedevent handling todocument-edit-actions-footer.tsxso 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.