Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Block editor: move is-navigate-mode class to WritingFlow #19868

Merged
merged 1 commit into from
Jan 24, 2020
Merged

Block editor: move is-navigate-mode class to WritingFlow #19868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/block-editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,6 @@ function BlockListBlock( {
'has-selected-ui': hasSelectedUI,
'has-warning': ! isValid || !! hasError || isUnregisteredBlock,
'is-selected': shouldAppearSelected && hasSelectedUI,
'is-navigate-mode': isNavigationMode,
'is-multi-selected': isMultiSelected,
'is-reusable': isReusableBlock( blockType ),
'is-dragging': isDragging,
Expand Down
4 changes: 2 additions & 2 deletions packages/block-editor/src/components/block-list/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@
}
}

&.is-navigate-mode::before {
.is-navigate-mode &::before {
border-color: $blue-medium-focus;
box-shadow: inset $block-left-border-width 0 0 0 $blue-medium-focus;

Expand Down Expand Up @@ -277,7 +277,7 @@
}

// Select tool/navigation mode shows the default cursor until an additional click edits.
&.is-navigate-mode {
.is-navigate-mode & {
cursor: default;
}

Expand Down
7 changes: 6 additions & 1 deletion packages/block-editor/src/components/writing-flow/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import { overEvery, find, findLast, reverse, first, last } from 'lodash';
import classnames from 'classnames';

/**
* WordPress dependencies
Expand Down Expand Up @@ -505,11 +506,15 @@ export default function WritingFlow( { children } ) {
}
}, [ hasMultiSelection, isMultiSelecting ] );

const className = classnames( 'block-editor-writing-flow', {
'is-navigate-mode': isNavigationMode,
} );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this is the right place for it.

If this is applied at this level, should the navigation styles be moved to the WritingFlow as well? For me ideally, this shouldn't be applied here unless the navigation mode is triggered here as well.

If the navigation mode is triggered from the block list (talking about the shortcuts), then maybe the style should stay where it is now but the class should also move to the BlockList.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which shortcut are you talking about? This component contains almost all logic for navigation mode (except for the breadcrumb to go back into Edit mode). It's used as the container to control the mode, so I feel like this component is appropriate for the class as well. I do agree that perhaps the CSS selectors should be moved as well, but I wasn't 100% sure about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm talking about "Enter" to enter "edit" mode and "Escape" to enter "navigation" mode.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ellatrix As an aside, I have a concern about all these refactorings we're doing to make the block.js smaller (classes and behavior). We often think about the Gutenberg use case in Core but we forget/neglect the packages and their reusable aspect. We should be making more playground stories to ensure we don't regress in these use cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escape to enter navigation mode is added by WritingFlow. Enter to enter edit mode is added by the breadcrumb within the block toolbar. Since WritingFlow forms a container around the blocks, it seems the most appropriate for the class (as opposed to the breadcrumb). I feel like the block and block list components don't have much to do with the mode other than some minor styling difference (which will even be removed in the future it seems).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I'm neglecting the packages here. Previously WritingFlow already handled a lot of navigation mode, so without it, it would be broken.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not blaming you or anyone :)

I know I have a tendency to forget about the playground and third-party usage sometimes and when I see things that could break that in other PRs, I wonder if I might not be the only one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know :) It's good to raise this. Perhaps we should add some tests for using the block editor on its own. I'll have a look into it at some point. That said, I don't think this PR in particular would break anything. If BlockList is used on its own, you can't really use navigation mode with WritingFlow. If WritingFlow is added, the class will be there as well.


// Disable reason: Wrapper itself is non-interactive, but must capture
// bubbling events from children to determine focus transition intents.
/* eslint-disable jsx-a11y/no-static-element-interactions */
return (
<div className="block-editor-writing-flow">
<div className={ className }>
<FocusCapture
ref={ focusCaptureBeforeRef }
selectedClientId={ selectedBlockClientId }
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/block/edit-panel/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@
}
}

.is-selected.is-navigate-mode .reusable-block-edit-panel {
.is-navigate-mode .is-selected .reusable-block-edit-panel {
border-color: $blue-medium-focus;
border-left-color: transparent;

Expand Down