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

Fix incorrectly displayed preview option for private post types #20604

Merged
merged 9 commits into from Mar 6, 2020

Conversation

@talldan
Copy link
Contributor

talldan commented Mar 3, 2020

Description

For private post types, the preview link is not normally displayed. The new preview menu isn't displaying this correctly though—while the Preview Externally link is (correctly) not displayed, the Open In New Tab icon is still being shown along with the surrounding menu group.

This PR fixes that to hide the menu group that contained the Preview externally link completely for non-viewable posts.

It also moves the Open In New Tab icon so that it's within the anchor element, whereas before it wasn't clickable.

How has this been tested?

Added e2e tests.

Manual Testing steps:

  1. Register a new private custom post type by adding the following php code:
function public_queryable_false_public_false_cpt() {
	$public_queryable_false_public_false = 'not_public';
	register_post_type(
		$public_queryable_false_public_false,
		array(
			'label'              => 'NotPublicQNotPublic',
			'show_in_rest'       => true,
			'public'             => false,
			'supports'           => array( 'title', 'editor', 'revisions' ),
			'show_ui'            => true,
			'show_in_menu'       => true,
		)
	);
}
add_action( 'init', 'public_queryable_false_public_false_cpt' );
  1. Add a new post of that type
  2. Type a post title
  3. View the preview dropdown menu
  4. Observe that the preview externally link isn't present
  5. Try adding a normal post and view the preview dropdown menu.
  6. Observe that the preview externally link is present

Screenshots

Before:
Screenshot 2020-03-03 at 5 23 46 pm

After:
Screenshot 2020-03-03 at 5 23 25 pm

Types of changes

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.
@talldan talldan requested review from ajitbohra, nerrad and ntwb as code owners Mar 3, 2020
@talldan talldan requested a review from tellthemachines Mar 3, 2020
@talldan talldan self-assigned this Mar 3, 2020
@github-actions

This comment has been minimized.

Copy link

github-actions bot commented Mar 3, 2020

Size Change: -16 B (0%)

Total Size: 863 kB

Filename Size Change
build/edit-post/index.js 91.3 kB +11 B (0%)
build/edit-post/style-rtl.css 8.69 kB -14 B (0%)
build/edit-post/style.css 8.69 kB -13 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 104 kB 0 B
build/block-editor/style-rtl.css 10.5 kB 0 B
build/block-editor/style.css 10.5 kB 0 B
build/block-library/editor-rtl.css 7.26 kB 0 B
build/block-library/editor.css 7.26 kB 0 B
build/block-library/index.js 115 kB 0 B
build/block-library/style-rtl.css 7.5 kB 0 B
build/block-library/style.css 7.51 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 5.75 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.03 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.36 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-site/index.js 4.64 kB 0 B
build/edit-site/style-rtl.css 2.48 kB 0 B
build/edit-site/style.css 2.48 kB 0 B
build/edit-widgets/index.js 4.42 kB 0 B
build/edit-widgets/style-rtl.css 2.59 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 325 B 0 B
build/editor/editor-styles.css 327 B 0 B
build/editor/index.js 43.8 kB 0 B
build/editor/style-rtl.css 3.98 kB 0 B
build/editor/style.css 3.98 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 734 B 0 B
build/format-library/index.js 7.11 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.48 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

Copy link
Contributor

tellthemachines left a comment

Thanks for fixing this! Tested with a custom post type, it's working well. Just one small CSS change suggested below 🙂

<>
{ __( 'Preview externally' ) }
<Icon
icon={ external }

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Mar 4, 2020

Contributor

Oooh I didn't know we could pass components into textContent 🤯

@@ -53,7 +53,7 @@

.editor-post-preview__button-external {
height: $button-size;
display: block;
display: flex;

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Mar 4, 2020

Contributor

If we add justify-content: space-between here we can remove the below .editor-post-preview__icon-external rule altogether.

This comment has been minimized.

Copy link
@talldan

talldan Mar 4, 2020

Author Contributor

Nice, that's a good little improvement 👍

@talldan talldan force-pushed the fix/stray-external-link-icon branch from 3a209d3 to 67d6ef4 Mar 4, 2020
const previewButtons = await page.$$(
'.editor-post-preview__button-resize'
);
expect( previewButtons.length ).toBe( 3 );

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Mar 4, 2020

Contributor

I'm wondering if we really need to check how many menu groups and resize buttons display in this test. Those things could change in the future, and this test shouldn't fail unless "Preview Externally" is actually showing.

This comment has been minimized.

Copy link
@talldan

talldan Mar 5, 2020

Author Contributor

I've changed it so that it now only checks for the link.

The reason I wrote it like that originally is that I wanted to be able to check for regressions of this bug, and the bug is that the menu group and icon are displayed. I wouldn't be able to add assertions for the menu group or icon without adding some kind of unique identifier to them.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Mar 5, 2020

Contributor

Oh, I see. I wonder if a snapshot test would be a better way of checking for that scenario. If we did change the markup, we'd be expecting snaphsots to fail anyway.

This comment has been minimized.

Copy link
@talldan

talldan Mar 5, 2020

Author Contributor

I personally feel like this is fine as it is. The test wouldn't be perfect, but it'd catch the most important failure, the presence of the link.

Snapshots for individual pieces of UI in end to end tests is a little outside of the norm, and while I'm not against it, it'd mean the snapshot needed to be updated for any minor unrelated change. That seems like it'd be a slower/more regular process than updating the element counts that were here previously.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Mar 5, 2020

Contributor

No worries!
I was thinking a snapshot as part of the unit tests, but that doesn't need to be part of this changeset.

@talldan talldan force-pushed the fix/stray-external-link-icon branch from efe11a5 to aa56f89 Mar 6, 2020
@talldan talldan merged commit e54323c into master Mar 6, 2020
3 checks passed
3 checks passed
build
Details
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@talldan talldan deleted the fix/stray-external-link-icon branch Mar 6, 2020
@github-actions github-actions bot added this to the Gutenberg 7.7 milestone Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.