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.

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

Restore Site Title's Level toolbar and remove obsolete code #24758

Merged

Conversation

david-szabo97
Copy link
Member

@david-szabo97 david-szabo97 commented Aug 24, 2020

Description

By mistake, we had two edit files. One at /edit.js and one at /edit/index.js. Looks like the Level toolbar functionality was added by this commit 988ba5d#diff-655f76535495d8ae1d82693b57a4ffba at/edit/index.js and later on /edit.js was created again by this commit 0446ed1#diff-655f76535495d8ae1d82693b57a4ffba . After this commit, we lost the Level Toolbar which was residing in the /edit/index.js. Since we had an edit folder and an edit.js file, the import was confusing since we had no idea whether the file or the folder being resolved.

This PR cleans up the mistake by removing the /edit.js and re-adding the Level toolbar.

Note:
We have a component at packages/block-library/src/heading/heading-level-dropdown.js which seems to be very similar to the one used by Site Title. Probably we should extract the component from the Heading block and reuse it here.

How has this been tested?

  1. Open Site Editor
  2. Add Site Title block
  3. Select for editing
  4. Shows a control for the heading level

Screenshots

image

Types of changes

New feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@ockham
Copy link
Contributor

ockham commented Aug 24, 2020

Thanks for digging!

It seems like the point of commit 0446ed1#diff-655f76535495d8ae1d82693b57a4ffba (PE #18361) that accidentally recreated /edit.js was to use the newly created Editable component (rather than RichText) to implement the SiteTitle block. This was subsequently changed in #21076 to use the PlainText component instead, and finally, in #22843, it was changed to use the RichText component again.

cc/ @ellatrix who made the Editable and PlainText changes, and @apeatling and @Addison-Stavlo who collaborated on #22843.

Also cc/ @epiqueras who originally added the Level toolbar in 988ba5d#diff-655f76535495d8ae1d82693b57a4ffba (#20361).

Edit: Specifically, I'm curious if we should change the block back to PlainText, since we don't want to offer rich-text editing for blocks like this anyway (instead, we only want to offer formatting that applies to the entire string regardless).

const [ title, setTitle ] = useEntityProp( 'root', 'site', 'title' );
const tagName = level === 0 ? 'p' : `h${ level }`;
const tagName = 0 === level ? 'p' : `h${ level }`;
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't enforce Yoda conditions in our JS (since we're enforcing typed comparisons -- === instead of == -- anyway).

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I copy-pasted it from the previous source and yeah, it felt strange to have Yoda conditions in JS. I'll revert this change since I don't like it either.

@ockham
Copy link
Contributor

ockham commented Aug 24, 2020

Note:
We have a component at packages/block-library/src/heading/heading-level-dropdown.js which seems to be very similar to the one used by Site Title. Probably we should extract the component from the Heading block and reuse it here.

That sounds like a great idea for a follow-up PR 👍

david-szabo97 and others added 2 commits August 25, 2020 09:13
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Thanks a lot! 🚢

@ockham ockham merged commit ffa22b8 into WordPress:master Aug 25, 2020
@github-actions github-actions bot added this to the Gutenberg 8.9 milestone Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants