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

Copy All Content button: Hide if is no content #16286

Merged
merged 1 commit into from
Jun 27, 2019

Conversation

Soean
Copy link
Member

@Soean Soean commented Jun 25, 2019

Description

The "Copy All Content" button should not be visible if there is no content.

Fixes #15500

hide-copy-button

@Soean Soean added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] UI Components Impacts or related to the UI component system labels Jun 25, 2019
@Soean Soean requested a review from afercia June 25, 2019 16:16
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Works well on my tests 👍

__( 'Copied!' ) :
__( 'Copy All Content' ) }
</ClipboardButton>
editedPostContent.length > 0 && (
Copy link
Member

Choose a reason for hiding this comment

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

I had some worries that editedPostContent may be null or undefined, but I checked the docs of the selector that ultimately is used, and it refers a string is always returned * @return {string} Post content.

@Soean Soean merged commit 2b061c5 into master Jun 27, 2019
@Soean Soean deleted the update/hide-copy-content-button branch June 27, 2019 13:58
@Soean Soean added this to the Gutenberg 6.1 milestone Jun 27, 2019
@aduth
Copy link
Member

aduth commented Jul 22, 2019

While it's arguably not useful if the post has no content, to avoid introducing inconsistency to the set of menu items available, I wonder if we would just leave it as it was, set the behavior to copy an empty string, and still show notices as if it were successful. Would that not still satisfy #15500, where the concern this change addresses is that there is no response to the button click?

__( 'Copied!' ) :
__( 'Copy All Content' ) }
</ClipboardButton>
editedPostContent.length > 0 && (
Copy link
Member

Choose a reason for hiding this comment

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

getEditedPostAttribute is a non-trivial selector, but it was previously more necessary because we did not conditionally render the element based on whether there is content or not.

With the addition of this condition, I wonder if we might consider to optimize the rendering to check whether there is content using the more-optimized isEditedPostEmpty (see inline comments for optimization specifics).

It could be applied using a series of higher-order components:

export default compose(
	withSelect( ( select ) => ( {
		hasEditedPostContent: ! select( 'core/editor' ).isEditedPostEmpty(),
	} ) ),
	ifCondition( ( { hasEditedPostContent } ) => hasEditedPostContent ),
	withSelect( ( select ) => ( {
		editedPostContent: select( 'core/editor' ).getEditedPostAttribute( 'content' ),
	} ) ),
	withDispatch( ( dispatch ) => {
		const {
			createNotice,
		} = dispatch( 'core/notices' );

		return {
			createNotice,
		};
	} ),
	withState( { hasCopied: false } )
)( CopyContentMenuItem );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't display "Copy All Content" in new, empty, posts
3 participants