-
Notifications
You must be signed in to change notification settings - Fork 374
[WIP] Hoist visual editor selection state to shell layer #1243
Conversation
…ell cares about them)
'focusedSteps' should only contain actions, such as 'triggers[0].actions[0]', however previous data could have trigger in it. Now, 'focusedId' is used as the old 'focusedSteps' which can fallback to 'triggers[i]' when no 'action' selected
|
WIP: add UT for reducer |
| apiClient.registerApi('onFocusEvent', focusEvent); | ||
| apiClient.registerApi('onFocusSteps', focusSteps); | ||
| apiClient.registerApi('onSelect', onSelect); | ||
| apiClient.registerApi('syncEditorState', syncEditorState); |
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.
Why is this called syncEditorState? We shouldn't be "syncing" anything, that suggested were mirroring state. The Shell data should hydrate the visual editor.
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.
Maybe its name should be onStateChange or onStoreChange: on each state change, notify container.
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.
There is a tricky problem here. Because of the iframe layer breaks change detection, state exchanging between Shell and editors has a really high cost (which resulted us to optimize it in #1106 ), so the normal pattern value + onChange no longer works fine. If we split 3 props into 3 pairs of 'value + onChange' to hydrate with Shell data, it will cause performance issue. Therefore, there is a onStateChange API for handling all state exchangings.
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.
According to comments below, we don't need the syncEditorState api. The interface will be:
<VisualEditor ... selection={selection} onChangeSelection={(selection) => shellApi.onChangeSelection(selection)} />
My only concern here is during a drag selection, the performance will be poor
| return state; | ||
| }; | ||
|
|
||
| const updateVisualEditorState: ReducerFunc = (state, { editorState }) => { |
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 don't like how you've defined this to suggest that a grouping of nodes is a concern of the visual editor. This isn't the case. The visual editor has a mechanism to group nodes and nothing more.
Here is why: If the visual editor gets redrawn for whatever reason, we still want to hold the grouped nodes in state.
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, This logic is supposed to be moved into lower level, at least down to ExtensionContainer / VisualEditorContainer.
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.
What about this one
<VisualEditor clipboardActions={actions} setClipboardActions={(actions) => shellApi.setClipboardActions(actions)} />
|
@cwhitten I realized the issue you talked about. My following plan:
|
|
Fix it in another approach, close this PR. |
Description
Previous implementation is a hack, replace it with a recommended solution.
Changes:
useReducerhooksyncEditorStateto enable state synchronization between Shell & Visual EditorvisualEditorActive(computed from editorState) to control if toolbar buttons (Copy/Cut/Delete) should be active.selectedwas passed asfocusedStepsbut it's actually a trigger (Form, Shell)Following works:
Apply this pattern to clipboard action, will potentialy persist copied actions in shell layer to enable copy/paste across dialog files.
Todos:
Task Item
#1170 - Resolve item 1
Type of change
Please delete options that are not relevant.
Checklist
Screenshots
Please include screenshots or gifs if your PR include UX changes.