-
Notifications
You must be signed in to change notification settings - Fork 374
[WIP] Hoist visual editor selection state to shell layer #1243
Changes from all commits
37e6960
55b04ce
9956b35
9711c1f
c97e3c8
207f6e3
be05ce6
1097a81
148a0f9
6943a02
cd84350
e5731d3
71c3ae6
c092144
0da4f02
9662748
14ec913
f1c300a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -224,6 +224,19 @@ const updatePublishStatus: ReducerFunc = (state, payload) => { | |
| return state; | ||
| }; | ||
|
|
||
| const updateVisualEditorState: ReducerFunc = (state, { editorState }) => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about this one |
||
| const arrayHasElements = (arr: any): boolean => Array.isArray(arr) && arr.length > 0; | ||
| const isVisualEditorActive = ({ selectedIds, focusedIds }): boolean => { | ||
| return arrayHasElements(selectedIds) || arrayHasElements(focusedIds); | ||
| }; | ||
|
|
||
| const visualEditorActive = isVisualEditorActive(editorState); | ||
| if (visualEditorActive !== state.visualEditorActive) { | ||
| state.visualEditorActive = visualEditorActive; | ||
| } | ||
| return state; | ||
| }; | ||
|
|
||
| export const reducer = createReducer({ | ||
| [ActionTypes.GET_PROJECT_SUCCESS]: getProjectSuccess, | ||
| [ActionTypes.GET_RECENT_PROJECTS_SUCCESS]: getRecentProjectsSuccess, | ||
|
|
@@ -261,4 +274,5 @@ export const reducer = createReducer({ | |
| [ActionTypes.PUBLISH_ERROR]: updatePublishStatus, | ||
| [ActionTypes.PUBLISH_BEGIN]: updatePublishStatus, | ||
| [ActionTypes.GET_ENDPOINT_SUCCESS]: updateRemoteEndpoint, | ||
| [ActionTypes.EDITOR_SYNCSTATE_VISUAL]: updateVisualEditorState, | ||
| } as { [type in ActionTypes]: ReducerFunc }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| const CLEAR_SELECTIONSTATE = 'VISUAL/SET_SELECTIONSTATE'; | ||
|
|
||
| export default function setSelectionState() { | ||
| return { | ||
| type: CLEAR_SELECTIONSTATE, | ||
| }; | ||
| } | ||
|
|
||
| export { CLEAR_SELECTIONSTATE }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import { BaseSchema } from 'shared'; | ||
|
|
||
| const SET_CLIPBOARD = 'VISUAL/SET_CLIPBOARD'; | ||
|
|
||
| export default function setClipboard(clipboardActions: BaseSchema[]) { | ||
| return { | ||
| type: SET_CLIPBOARD, | ||
| payload: { | ||
| actions: clipboardActions, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| export { SET_CLIPBOARD }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| const SET_DRAGSELECTION = 'VISUAL/SET_DRAGSELECTION'; | ||
|
|
||
| export default function setSelection(seletedIds: string[]) { | ||
| return { | ||
| type: SET_DRAGSELECTION, | ||
| payload: { | ||
| ids: seletedIds, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| export { SET_DRAGSELECTION }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| const SET_FOCUSSTATE = 'VISUAL/SET_FOCUSSTATE'; | ||
|
|
||
| export default function setFocusState(focusedIds: string[]) { | ||
| return { | ||
| type: SET_FOCUSSTATE, | ||
| payload: { | ||
| ids: focusedIds, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| export { SET_FOCUSSTATE }; |
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.Uh oh!
There was an error while loading. Please reload this page.
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
onStateChangeoronStoreChange: 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
onStateChangeAPI for handling all state exchangings.Uh oh!
There was an error while loading. Please reload this page.
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
syncEditorStateapi. The interface will be:My only concern here is during a drag selection, the performance will be poor