#5618 ketcher doesnt trigger change event in macromolecule mode #6012
Conversation
Drimodaren
changed the title
|
|
Drimodaren
changed the title
…n-macromolecule-mode
There was a problem hiding this comment.
I don't think this and many of the following screenshot updates are correct to be honest – there shouldn't be any structure duplication I guess
| @@ -25,6 +26,7 @@ export class EditorHistory { | |||
| historyStack: Command[] | [] = []; | |||
| historyPointer = 0; | |||
| editor: CoreEditor | undefined; | |||
| ketcherInstance; | |||
There was a problem hiding this comment.
As we discussed, you don't need this class property for now
| } else { | ||
| this.event.change.dispatch(action); | ||
| ketcherProvider.getKetcher().changeEvent.dispatch(action); |
There was a problem hiding this comment.
For a bit of readability I'd prefer to have some variable assigned to ketcher instance and it being reused as long as function uses ketcher more than once
| @@ -40,7 +40,9 @@ type Data = { | |||
|
|
|||
| export function customOnChangeHandler(action, handler) { | |||
| const data: Data[] = []; | |||
|
|
|||
| if (!action) { | |||
There was a problem hiding this comment.
Let's prepare this function a bit for some future adjustments. Let it be wrapper for both micro and macro 'changes' event handler and we will have two separate functions for micro and macro event handlers. The former will have all the existing functionality – preparing the data based on passed action and invoking the handler with this data. The latter will just have invoking the handler with empty arguments list – just what you've added here
Then, based on the currently active editor (you can use window.isPolymerEditorTurnedOn) just invoke either the first or the second function.
| @@ -40,7 +40,9 @@ type Data = { | |||
|
|
|||
| export function customOnChangeHandler(action, handler) { | |||
There was a problem hiding this comment.
For a fail safe check I'd add type for action parameter – let it be Action | undefined for now, as we could see, it's possible to trigger event without any data and it will silently fail on the following forEach call
| @@ -557,6 +557,7 @@ class Editor implements KetcherEditor { | |||
| } | |||
|
|
|||
| undo() { | |||
| const ketcherProviderEditor = ketcherProvider.getKetcher().changeEvent; | |||
There was a problem hiding this comment.
It is rather event instance than editor instance, I'd change variable name
There was a problem hiding this comment.
Maby ketcherChangeEvent?
How the feature works? / How did you fix the issue?
(Screenshots, videos, or GIFs, if applicable)
Check list
#1234 – issue name