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.

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

Toolbar: Refactor to simplify block toolbar rendering #4412

Merged
merged 2 commits into from Jan 11, 2018

Conversation

Projects
None yet
2 participants
@aduth
Member

aduth commented Jan 11, 2018

This pull request seeks to refactor the EditorBlockToolbar component to simplify its rendering logic. Specifically, there is an ESLint rule disabling which is unnecessary, and the component renders a wrapper with a fragment of elements only when editing a block visually; this has been simplified to render nothing from the component at all unless editing a block visually, assuming that an empty wrapper is not necessary.

Testing instructions:

Verify that there are no regressions in the display and use of the block editor, contextually or bound to the top toolbar, and that the toolbar only displays when editing a block visually.

@aduth aduth added the Chrome label Jan 11, 2018

@@ -16,22 +16,16 @@ import BlockSwitcher from '../block-switcher';
import { getBlockMode, getSelectedBlock } from '../../store/selectors';
function BlockToolbar( { block, mode } ) {
if ( ! block || ! block.isValid ) {
if ( ! block || ! block.isValid || mode !== 'visual' ) {

This comment has been minimized.

@youknowriad

youknowriad Jan 11, 2018

Contributor

Granted the issue was here before but I think the mode should not be checked here but more in the edit-post folder where the BlockToolbar is inserted (as it's layout specific)

@youknowriad

youknowriad Jan 11, 2018

Contributor

Granted the issue was here before but I think the mode should not be checked here but more in the edit-post folder where the BlockToolbar is inserted (as it's layout specific)

This comment has been minimized.

@aduth

aduth Jan 11, 2018

Member

Granted the issue was here before but I think the mode should not be checked here but more in the edit-post folder where the BlockToolbar is inserted (as it's layout specific)

Are you referring to the "Visual vs. Text" distinction on the post editor layout, or (as implemented here) the "Edit as Visual vs Edit as HTML" distinction on a block?

@aduth

aduth Jan 11, 2018

Member

Granted the issue was here before but I think the mode should not be checked here but more in the edit-post folder where the BlockToolbar is inserted (as it's layout specific)

Are you referring to the "Visual vs. Text" distinction on the post editor layout, or (as implemented here) the "Edit as Visual vs Edit as HTML" distinction on a block?

This comment has been minimized.

@youknowriad

youknowriad Jan 11, 2018

Contributor

Oh sorry! I misread the PR. You're totally right and the block mode makes sense here.

@youknowriad

youknowriad Jan 11, 2018

Contributor

Oh sorry! I misread the PR. You're totally right and the block mode makes sense here.

This comment has been minimized.

@aduth

aduth Jan 11, 2018

Member

Oh sorry! I misread the PR. You're totally right and the block mode makes sense here.

I mean, it's good to highlight the confusion at least caused by overloading the use of the word Visual 😆

@aduth

aduth Jan 11, 2018

Member

Oh sorry! I misread the PR. You're totally right and the block mode makes sense here.

I mean, it's good to highlight the confusion at least caused by overloading the use of the word Visual 😆

@youknowriad

LGTM 👍

@aduth aduth merged commit c2d3fe5 into master Jan 11, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aduth aduth deleted the refactor/block-toolbar branch Jan 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment