Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upUse the block editor store instead of the editor one #13105
Conversation
youknowriad
added
the
Framework
label
Dec 27, 2018
youknowriad
added this to the Future: Phase 2 milestone
Dec 27, 2018
youknowriad
self-assigned this
Dec 27, 2018
youknowriad
requested a review
from WordPress/gutenberg-core
Dec 27, 2018
This comment has been minimized.
This comment has been minimized.
It looks like settings should be extracted to their own module and maybe even divided into sections:
Otherwise, those changes look good. |
youknowriad
force-pushed the
try/reusable-block-editor-module
branch
from
f19c293
to
38d373d
Jan 8, 2019
youknowriad
force-pushed the
try/reusable-block-editor-module
branch
2 times, most recently
from
e8915a1
to
6cc6c8a
Jan 18, 2019
youknowriad
modified the milestones:
Future,
WordPress 5.x,
5.0 (Gutenberg)
Jan 22, 2019
youknowriad
modified the milestones:
5.0 (Gutenberg),
5.1 (Gutenberg)
Feb 4, 2019
youknowriad
force-pushed the
try/reusable-block-editor-module
branch
3 times, most recently
from
9967510
to
d4832c4
Feb 4, 2019
noisysocks
modified the milestones:
5.1 (Gutenberg),
5.2 (Gutenberg)
Feb 18, 2019
aduth
force-pushed the
try/reusable-block-editor-module
branch
from
990aa38
to
c14cbec
Feb 18, 2019
youknowriad
force-pushed the
try/reusable-block-editor-module
branch
from
9dea161
to
4846f03
Feb 19, 2019
aduth
force-pushed the
try/reusable-block-editor-module
branch
from
af0cdd0
to
208991f
Feb 19, 2019
youknowriad
force-pushed the
try/reusable-block-editor-module
branch
6 times, most recently
from
4cda75f
to
01a154c
Feb 21, 2019
youknowriad
force-pushed the
update/use-block-editor-store
branch
from
b8cac33
to
ae6f86d
Feb 22, 2019
youknowriad
requested a review
from
aduth
as a
code owner
Feb 22, 2019
youknowriad
requested review from
nosolosw,
notnownikki,
ntwb,
Soean and
talldan
as
code owners
Feb 22, 2019
youknowriad
changed the base branch from
try/reusable-block-editor-module
to
master
Feb 22, 2019
This comment has been minimized.
This comment has been minimized.
Now that the generic block editor module is merged, this is ready to be reviewed. |
youknowriad
added some commits
Dec 27, 2018
youknowriad
force-pushed the
update/use-block-editor-store
branch
from
1560d1b
to
f5e3135
Feb 22, 2019
nerrad
reviewed
Feb 22, 2019
@@ -56,7 +56,8 @@ function HeaderToolbar( { hasFixedToolbar, isLargeViewport, showInserter } ) { | |||
export default compose( [ | |||
withSelect( ( select ) => ( { | |||
hasFixedToolbar: select( 'core/edit-post' ).isFeatureActive( 'fixedToolbar' ), | |||
showInserter: select( 'core/edit-post' ).getEditorMode() === 'visual' && select( 'core/editor' ).getEditorSettings().richEditingEnabled, | |||
// This setting (richEditingEnabled) should not live in the block editor's setting. |
This comment has been minimized.
This comment has been minimized.
nerrad
Feb 22, 2019
Contributor
What's the reasoning behind this? Is richEditingEnabled
something that is a global setting for all block editor instances? Is there potential for it being something that might only be desired for specific editor instances (which might override a global)?
This comment has been minimized.
This comment has been minimized.
youknowriad
Feb 22, 2019
Author
Contributor
The reasoning is that the when you use the block editor module, you already know that the rich text is enabled. It's more something the parent module should care about.
getReferenceByDistinctEdits, | ||
isAutosavingPost, | ||
} = select( 'core/editor' ); | ||
|
||
const { autosaveInterval } = getEditorSettings(); | ||
// This settings should not live in the block editor. |
This comment has been minimized.
This comment has been minimized.
nerrad
Feb 22, 2019
Contributor
Similar line of questioning here, is this because autosaveInternal
is more of a global setting for all editor instances?
const { | ||
getBlockSelectionStart, | ||
} = select( 'core/block-editor' ); | ||
|
This comment has been minimized.
This comment has been minimized.
@@ -161,12 +161,16 @@ const EnhancedVisualEditorGlobalKeyboardShortcuts = compose( [ | |||
}; | |||
} ), | |||
withDispatch( ( dispatch ) => { | |||
// This component should probably be split into to | |||
// A block editor specific one and a post editor one. |
This comment has been minimized.
This comment has been minimized.
nerrad
Feb 22, 2019
Contributor
What's the reasoning for this? Is it because of the undo/redo in the component?
This comment has been minimized.
This comment has been minimized.
youknowriad
Feb 22, 2019
Author
Contributor
yes, ideally a user of the block editor module should have the block shortcuts enabled by default and would be able to add other shortcuts if needed (undo/redo in that case)
This comment has been minimized.
This comment has been minimized.
gziolo
Feb 25, 2019
Member
Nit: there is A
in the middle of the sentence. However more importantly, do we want to add TODO:
items for all comments introduced in this PR? If yes, ideally I would see them with the referenced issue to make it easier to remove later.
} = select( 'core/block-editor' ); | ||
|
||
// The title should be removed from the inserter | ||
// or replaced by a prop passed to the inserter. |
This comment has been minimized.
This comment has been minimized.
nerrad
Feb 22, 2019
Contributor
Ya this is used when the isMobile
is true for the Popover. So is the idea here it shouldn't be tightly coupled to a post editor title? Maybe there should be a generic block-editor title that defaults to the post title if present?
This comment has been minimized.
This comment has been minimized.
const { getPostType } = select( 'core' ); | ||
|
||
// This setting should not live in the block-editor module. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
youknowriad
Feb 22, 2019
Author
Contributor
No that's correct, the templates is something post related, and the block editor shouldn't be aware of it (it doesn't need it)
@@ -15,7 +15,8 @@ function PostFormatCheck( { disablePostFormats, ...props } ) { | |||
|
|||
export default withSelect( | |||
( select ) => { | |||
const editorSettings = select( 'core/editor' ).getEditorSettings(); | |||
// This setting should not live in the block-editor's store. | |||
const editorSettings = select( 'core/block-editor' ).getEditorSettings(); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
const { getPostType } = select( 'core' ); | ||
return { | ||
isLocked: isPostLocked(), | ||
isTakeover: isPostLockTakeover(), | ||
user: getPostLockUser(), | ||
postId: getCurrentPostId(), | ||
// This setting should not live in the block-editor's store. | ||
postLockUtils: getEditorSettings().postLockUtils, |
This comment has been minimized.
This comment has been minimized.
@@ -1104,7 +1104,9 @@ const RichTextContainer = compose( [ | |||
}; | |||
} ), | |||
withSelect( ( select ) => { | |||
const { canUserUseUnfilteredHTML, isCaretWithinFormattedText } = select( 'core/editor' ); | |||
// This should probably be moved to the block editor settings. | |||
const { canUserUseUnfilteredHTML } = select( 'core/editor' ); |
This comment has been minimized.
This comment has been minimized.
@@ -57,7 +57,7 @@ function TableOfContentsPanel( { headingCount, paragraphCount, numberOfBlocks } | |||
} | |||
|
|||
export default withSelect( ( select ) => { | |||
const { getGlobalBlockCount } = select( 'core/editor' ); | |||
const { getGlobalBlockCount } = select( 'core/block-editor' ); |
This comment has been minimized.
This comment has been minimized.
nerrad
Feb 22, 2019
Contributor
getGlobalBlockCount
sounds like something that should consider ALL loaded editor instances in a view (as opposed to getBlockCount
which just would consider count of blocks for just that editor instance.
This comment has been minimized.
This comment has been minimized.
youknowriad
Feb 22, 2019
Author
Contributor
I think the difference at the moment between the two is whether to count the inner blocks or not.
This comment has been minimized.
This comment has been minimized.
nerrad
Feb 22, 2019
Contributor
Hmm unfortunate name then. It does present potential naming collision/confusion if in the future there's some way of counting blocks for all editor instances in a view.
This comment has been minimized.
This comment has been minimized.
youknowriad
Feb 22, 2019
Author
Contributor
What kind of use-case do you imagine for having a count of all blocks across editors?
This comment has been minimized.
This comment has been minimized.
nerrad
Feb 22, 2019
•
Contributor
None I can think of offhand besides more of an informational stats like view to the end-user. I don't think this is critical to change, but the fact that it's not immediately apparent what this function represents is a bit unfortunate (the whole "naming is hard" kind of thing ;) ). "Global" is a fairly broad term.
gziolo
reviewed
Feb 25, 2019
It looks like this PR moves all editor settings to |
@@ -37,7 +37,6 @@ class ErrorBoundary extends Component { | |||
// used here because it (a) reduces the chance of data loss in the | |||
// case of additional errors by performing a direct retrieval and | |||
// (b) avoids the performance cost associated with unnecessary | |||
// content serialization throughout the lifetime of a non-erroring |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@@ -161,12 +161,16 @@ const EnhancedVisualEditorGlobalKeyboardShortcuts = compose( [ | |||
}; | |||
} ), | |||
withDispatch( ( dispatch ) => { | |||
// This component should probably be split into to | |||
// A block editor specific one and a post editor one. |
This comment has been minimized.
This comment has been minimized.
gziolo
Feb 25, 2019
Member
Nit: there is A
in the middle of the sentence. However more importantly, do we want to add TODO:
items for all comments introduced in this PR? If yes, ideally I would see them with the referenced issue to make it easier to remove later.
This comment has been minimized.
This comment has been minimized.
I tested this PR with the following debugging change: diff --git a/packages/editor/src/store/selectors.js b/packages/editor/src/store/selectors.js
index c3147abda..a9d43d3a8 100644
--- a/packages/editor/src/store/selectors.js
+++ b/packages/editor/src/store/selectors.js
@@ -1096,6 +1096,7 @@ export function __unstableIsEditorReady( state ) {
function getBlockEditorSelector( name ) {
return createRegistrySelector( ( select ) => ( state, ...args ) => {
+ console.log( name );
return select( 'core/block-editor' )[ name ]( ...args );
} );
} and couldn't see any message logged to JS console. |
This comment has been minimized.
This comment has been minimized.
The change is ready to be pushed, just waiting for this merge :)
#14043 this issue is listing all the follow-up items for the generic block editor module. |
gziolo
approved these changes
Feb 25, 2019
Code wise this looks and tests well. Let's move forward and iterate further as discussed in the comments. |
youknowriad commentedDec 27, 2018
This PR builds on top of the reusable block editor module PR. #13088
Instead of using the proxy selectors/actions added for backward compatibility purpose, this PR replaces the usage of the
core/editor
store withcore/block-editor
when necessary.It also highlights some places where some usage of these stores is not good and should be refactored. I added some comments to explain the refactorings needed. (Small ones)
Otherwise, this PR is pretty straight-forward.
The next step would be to do these refactorings and move the block editor specific components to the block editor module.