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

Do not show separator in block settings menu if template is locked #7751

Merged
merged 2 commits into from
Aug 6, 2018
Merged

Do not show separator in block settings menu if template is locked #7751

merged 2 commits into from
Aug 6, 2018

Conversation

nfmohit
Copy link
Member

@nfmohit nfmohit commented Jul 6, 2018

Description

This PR addresses #7692 which reports the display of the separator in the block settings menu if the template is locked.

This change determines if the template is locked and only displays the separator if it isn't.

How has this been tested?

This has been tested following these steps:

  1. A template of a post type was locked according to this documentation.
  2. It was made sure that the separator doesn't show up in the "More Options" menu, i.e. the block settings menu for that post type with a locked template.

This was tested in WP 4.9.7, Gutenberg 3.1.1, Apache server with PHP 7.2.0 and MySQL 5.6.34. According to initial tests, the code doesn’t seem to affect any other areas.

Screenshots

pull-7692

Types of changes

This PR just imports and adds the isLocked constant and binds the .editor-block-settings-menu__separator element within a condition so that it is displayed only when isLocked is false.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Nice work here. I have a reservations about the approach. Because BlockRemoveButton is responsible for not rendering itself based on the lock state of the editor, we're introducing some inconsistency here in that BlockSettingsMenu is partially aware of this behavior. I think either we should also use isLocked in BlockSettingsMenu to decide whether to render BlockRemoveButton, or find another way which doesn't rely on knowing that the template is locked. With #7692, one of the ideas I had in mind was that the mere presence of other items in the list should determine whether the separator is visible. The only thing that comes to mind at the moment, aside from some approaches in React that may be fragile in relying on component lifecycle and inspecting the component's own children, is the CSS adjacent sibling selector to define the border on the node following the separator.

@nfmohit
Copy link
Member Author

nfmohit commented Jul 8, 2018

Thank you so much for the review @aduth ❤️!

I understand what you mean. Could you look at the following code?

.editor-block-settings-menu__popover .editor-block-settings-menu__separator:nth-last-child(1) {
    display: none;
}

What if we apply the above CSS to the block settings menu instead of determining if the template is locked? This'll check if the separator is the last child in the node and if that is true will not display itself. Does that sound like a solution?

Looking forward to your feedback. Thank you!

@aduth
Copy link
Member

aduth commented Jul 23, 2018

@nfmohit-wpmudev Apologies for the delay in my response. I like this idea. Could it just be the :last-child pseudo-class? Maybe also including an inline code comment explaining the intent with the selector, since it won't be very obvious at a glance.

@nfmohit
Copy link
Member Author

nfmohit commented Jul 24, 2018

@aduth No worries and thank you so much for the review ❤️. :last-child sounds much cleaner. I've added a new commit restoring the older changes and adding the CSS (with the comment).

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Looks great 👍

@aduth aduth added the General Interface Parts of the UI which don't fall neatly under other labels. label Aug 6, 2018
@aduth aduth merged commit 22ba56f into WordPress:master Aug 6, 2018
@nfmohit
Copy link
Member Author

nfmohit commented Aug 6, 2018

Thank you so much @aduth

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants