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

PrePublishPanel: suggest tags and postformat #8235

Merged
merged 30 commits into from
Aug 13, 2018

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Jul 26, 2018

Related #7426

This PR aims to include the Tags and the PostFormat components in the pre-publish panel:

  • Tags component is shown only if the user hasn't tagged the post yet.
  • PostFormat is shown if the suggested PostFormat is different from the current one.

How to test

  • Create a new post.
  • Add a block image (doesn't need to have any actual image).
  • Click Publish.
  • The expected behavior is that the pre-publish panel includes both the Tags and the PostFormat components to prompt the user to add them. Both should be collapsed. See:
Closed Opened
pre-publish-panel-copy-long-closed pre-publish-panel-copy-toffumatt

Test variations of the above: with the document sidebar closed (as this is when tags are fetched), with tags and image as the current post format, etc. The suggested post format is shown below the current post format, if there is any suggestion - typing wp.data.select('core/editor').getSuggestedPostFormat() in the console will give you the same result (or null if none is to be suggested).

TODO

  • Make text translatable.
  • Try a more conversational post format suggestion (see Use Pre-Publish Panel for Tags and Post Format #7426 (comment))
  • Not shown conditions:
    • Post already has some tag applied.
    • It's a page, not a post (more generally, if the post type allows tags).
  • Fix:
    • Don't crash if tags weren't fetched yet.
    • Panel is hidden after adding one tag in the pre-publish panel.

Open questions

  • Should we suggest only tags or any available taxonomy? We're going to go with tags as a starter.

@oandregal oandregal self-assigned this Jul 26, 2018
@oandregal oandregal added the [Status] In Progress Tracking issues with work in progress label Jul 26, 2018
@oandregal oandregal changed the title Suggest adding tags in PrePublishPanel Suggest adding tags in PrePublishPanel is no tag was added Jul 26, 2018
@oandregal
Copy link
Member Author

To the open question regarding whether to include only tags or all available taxonomies in the PrePublishPanel: I lean towards adding any taxonomy that has at least one term (is there any other heuristic we can use to infer which taxonomies are relevant for a given site?).

@chrisvanpatten
Copy link
Member

To the open question regarding whether to include only tags or all available taxonomies in the PrePublishPanel: I lean towards adding any taxonomy that has at least one term (is there any other heuristic we can use to infer which taxonomies are relevant for a given site?)

I would just make it tags or categories, but nothing else by default. Perhaps make it filterable so developers can opt their taxonomies in, but I think it's dangerous to assume all other taxonomies are relevant or worthy of a notification.

@sarahmonster
Copy link
Member

I think the original concept here (cc @mtias) was to show the tags module only when no tags have been added to the post—so it wouldn't appear at all times, but it's there to serve as a reminder if the user has forgotten to add any tags.

I’m not sure categories are as relevant to the modern #hashtag web—and I suspect people are often unsure of the difference between the two—so I think starting with tags would be sensible. (I see a lot of people who don't even use categories, but I'm not sure if that's because the categories are less surfaced in the UI or because they're just an advanced feature.)

The ideal would probably be if we suggested some tags based on the post content and/or the users’ commonly used tags, but that’s obviously a bit more complicated to implement. Failing that, suggesting the most commonly-used tags (maybe three?) would be a good start.

I’m not certain if tag suggestions are better in the pre-publish panel or in the post-publish panel, since they aren't really critical to the pre-publish experience, per se. Before publishing, I want to make sure the date is right, check on anything that's going to automatically send out... basically make sure I have control over everything that happens "immediately." Adding tags can help users find your post, but ultimately it doesn't matter much if you add them before or after publishing the post.

That said, users are likely to be a bit more focussed at the pre-publish stage, and since we currently aren't bombarding them with information and Things to Check at this stage, having them here for the time being probably works. If this panel ends up being used heavily by plugins, we may want to consider shifting them to the post-publish panel in order to reduce noise and make publishing as frictionless as possible.

@oandregal oandregal force-pushed the add/tags-to-prepublishpanel branch 2 times, most recently from 47be367 to e927c9a Compare August 1, 2018 16:04
@oandregal oandregal removed the [Status] In Progress Tracking issues with work in progress label Aug 1, 2018
@oandregal
Copy link
Member Author

@sarahmonster this is ready for design review.

cc @gziolo for code review.

@oandregal oandregal requested a review from gziolo August 1, 2018 17:16
@gziolo gziolo requested review from a team and youknowriad August 2, 2018 07:36
constructor( props ) {
super( props );
this.state = {
hadTags: props.hasTags,
Copy link
Member

Choose a reason for hiding this comment

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

Can you help me understand why it is not possible to set state only once on initial load and keep it this way for the rest of interaction with this panel? Checks like:

if ( currentHadTags === undefined && hasTags !== undefined ) {

indicate that it can be undefined for some reasons. If that is the reason, can we postpone mounting this component until this data is fetched?

Copy link
Member Author

@oandregal oandregal Aug 2, 2018

Choose a reason for hiding this comment

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

That's explained it in the long getDerivedStateFromProps comment. TLDR is: tags may be not fetched yet at this point. How to test this behavior:

  • Create a new post.
  • Make sure the sidebar document hasn't been opened. If it is, collapse it, and reload the page.
  • Click the publish button. At this point, the pre-publish panel will be opened but the taxonomies aren't fetched yet (because the sidebar document is closed).

I'd love to get rid of this complexity, but Gutenberg fetching mechanisms are still fuzzy for me. Any suggestion about code examples I should look at for inspiration and learn-up?

Copy link
Member Author

Choose a reason for hiding this comment

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

Found a way to do this by using the ifCondition HOC component suggested by Riad, see e7dde0f.

const tags = select( 'core' ).getTaxonomy( 'post_tag' );
const terms = tags ? select( 'core/editor' ).getEditedPostAttribute( tags.rest_base ) : [];
return {
hasTags: tags && terms && terms.length > 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this condition be something like that instead:

const tagTaxonomy = select( 'core' ).getTaxonomy( 'post_tag' );
const shouldShowMoreTags = tagTaxonomy && ( ! select( 'core/editor' ).getEditedPostAttribute( tags.rest_base ) || select( 'core/editor' ).getEditedPostAttribute( tags.rest_base ).length ===0 ) &&  tagTaxonomy.types.some( ( type ) => type === postType );

Something in that vein?

And I guess we can leverage the ifCondition HigherOrderComponent to avoid the MaybeTagsPanel component entirely?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the ifCondition suggestion: I've used that in e66d09d to prevent the component from mounting if some conditions aren't met.

In this case, the internal component state is useful: we need a place to encode whether the post had tags when the user clicked publish. We can't use the hasTags prop because it'll become true if the user adds a new tag in the pre-publish panel, causing a re-render that hides the tags component, and so preventing the user from adding more than one tag.

I've also refactored the logic to make the assumptions more explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ok, I get it now and in that case, I think you were right originally and now it can fail if the taxonomy is not loaded initially (in theory, because in practice, it's already there).

But I'm wondering if the hasTags condition makes sense at all, why can't we just always show the tags there if the post supports it? It seems better for me even if I already entered tags to check them on pre-publish. It also makes the code way simpler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh ok, I get it now and in that case, I think you were right originally and now it can fail if the taxonomy is not loaded initially (in theory, because in practice, it's already there).

It's still correct. Note that if the taxonomy is not loaded (which do happen when the user clicks the publish button without the sidebar being opened first), the areTagsFetched will be false. e66d09d decoupled two separate concerns that were previously encoded in only one variable: whether or not the tags taxonomy has been fetched (areTagsFetched) and if the post has tags (hasTags).

It seems better for me even if I already entered tags to check them on pre-publish. It also makes the code way simpler.

That's a design decision that I defer to Matías and Sarah. It seems sensible to limit the amount of info in the pre-publish panel - we don't want to have here info that's already filled in the sidebar by the user.

@oandregal
Copy link
Member Author

oandregal commented Aug 2, 2018

I was thinking of adding the PostFormat component in a different PR but figured it'd better to include it here as well - see cd57766. It helps to review the design changes holistically (things like whether the panels should be opened/closed, the space they take, and so on). I also hope this minimizes code review time.

@oandregal oandregal changed the title Suggest adding tags in PrePublishPanel is no tag was added PrePublishPanel: suggest tags and postformat Aug 2, 2018
@@ -41,6 +43,8 @@ function PostPublishPanelPrepublish( {
] }>
<PostSchedule />
</PanelBody>
<MaybePostFormatPanel />
Copy link
Member

Choose a reason for hiding this comment

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

Have you checked how <PostSchedule / or <PostVisibility /> are implemented?

https://github.com/WordPress/gutenberg/tree/master/packages/editor/src/components/post-schedule
https://github.com/WordPress/gutenberg/tree/master/packages/editor/src/components/post-visibility

They contain two parts the components which render the panel's body and a wrapper component with suffix Check. It would be nice to align.

@youknowriad, why we don't wrap those components with checks in the publish panel? We do it in the sidebar:
https://github.com/WordPress/gutenberg/blob/master/edit-post/components/sidebar/post-schedule/index.js#L15-L35

Copy link
Contributor

Choose a reason for hiding this comment

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

Because it's already taken care of in the component itself. In the sidebar, we don't want the extra panels if the check is false, here, there's no extra wrappers added.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can extract the ifCondition to a Check component if that's preferred. We use a variety of patterns through the codebase, though, so unless we want to consolidate towards using a particular one I'm happy as it is.

@oandregal
Copy link
Member Author

Changes introduced in 46a181a

  • gave dashicon a size of 18 (it was 20 previously)
  • added the yellow color
  • updated copy
Before After
post-format-and-tags tips-before-after-18

@oandregal
Copy link
Member Author

oandregal commented Aug 2, 2018

Changes in b4de407 make the post format suggestion more conversational:

Before After
pre-publish-panel-open pre-publish-panel-conversational

@oandregal
Copy link
Member Author

oandregal commented Aug 2, 2018

7ffe43a, 06f2b7f and 73b6f3b update the copy text:

  • Text below the title was consolidated to complement why that's important in both blocks.
  • Format post type: due to translations corcerns, call to action and suggested post format should be different chunks of text (not mixed).
Before After
pre-publish-panel-conversational pre-publish-panel-copy-translations

@sarahmonster sarahmonster added the Needs Copy Review Needs review of user-facing copy (language, phrasing) label Aug 3, 2018
Copy link
Member

@sarahmonster sarahmonster left a comment

Choose a reason for hiding this comment

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

I know I suggested it, but I'm definitely not crazy about the yellow lightbulb. The yellow is way too much, and the icon feels a bit awkward. We could try replicating the tips that are used elsewhere (ie, a blue dot), but I think that may be a bit confusing. Let's just nix the icon altogether for now.

Your readers will be able to find your posts more easily.

Suggestion:

This will help readers find your posts.

I also don't seem to be able to get the post format suggestion to display:

PostFormatSuggested@http://localhost:8888/wp-content/plugins/gutenberg/build/editor/index.js?ver=1533257225:23894:1
yh@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:95:430
lg@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:120:88
mg@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:120:386
gc@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:127:202
vb@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:126:230
ub@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:126:65
wg@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:137:182
Ze@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:41:24
EventListener.handleEvent*p@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:40:392
W@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:49:377
qf@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:52:256
zh@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:108:355
kg@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:118:263
lg@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:120:121
mg@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:120:386
gc@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:127:202
vb@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:126:230
ub@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:126:65
zd@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:124:449
ra@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:123:319
enqueueSetState@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react-dom.min.82e21c65.js:189:231
q.prototype.setState@http://localhost:8888/wp-content/plugins/gutenberg/vendor/react.min.ab6b06d4.js:18:428
subscribe/this.unsubscribe<@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:7467:13
globalListener/<@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:8027:14
globalListener@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:8026:5
registerReducer/<@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:8068:9
dispatch@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:6065:7
dispatch@http://localhost:8888/wp-admin/post-new.php:1:38397
_callee2$@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:8217:21
tryCatch@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:6338:37
invoke@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:6564:22
defineIteratorMethods/</prototype[method]@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:6390:16
asyncGeneratorStep@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:437:16
_next@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:459:9
run@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:3228:22
notify/<@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:3245:30
flush@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:1938:9
MutationCallback*./node_modules/core-js/library/modules/_microtask.js/module.exports@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:1957:5
./node_modules/core-js/library/modules/es6.promise.js@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:3166:17
__webpack_require__@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:21:12
./node_modules/core-js/library/fn/promise.js@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:1037:1
__webpack_require__@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:21:12
./node_modules/@babel/runtime/core-js/promise.js@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:203:18
__webpack_require__@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:21:12
./node_modules/@babel/runtime/helpers/asyncToGenerator.js@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:433:16
__webpack_require__@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:21:12
./packages/data/build-module/registry.js@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:7828:97
__webpack_require__@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:21:12
./packages/data/build-module/default-registry.js@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:7503:67
__webpack_require__@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:21:12
./packages/data/build-module/index.js@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:7648:75
__webpack_require__@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:21:12
this.wp.data<@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:70:18
@http://localhost:8888/wp-content/plugins/gutenberg/build/data/index.js?ver=1533257225:2:11

Side note: this looks pretty busy right now, but I think could be improved a great deal by merging #7879, which seems to have gotten stuck in limbo. I'll look into pushing that change through in a more incremental way.

@oandregal
Copy link
Member Author

Changes in b379a78, 0fcc49e, and 1f20a64

Before After
pre-publish-panel-copy-translations pre-publish-panel-no-icon

@michelleweber
Copy link

On the tags, I'd love to add just a few more words that add detail on the what/why of tags:

Enter a few words that describe your post's subject to help interested readers find it.

And little tweaks on post format:

Your theme uses post formats -- styling tweaks that complement different kinds of content. The Image format would be great for this post.

@oandregal
Copy link
Member Author

Implemented Michelle's suggestions at c29f3b0

pre-publish-panel-copy-text

@oandregal
Copy link
Member Author

@sarahmonster @tofumatt can we 🚢 this or is there anything else we should address?

@tofumatt
Copy link
Member

ah, sorry, if you could re-request me for review once things are ready to check again I'll see it in my review queue and have a look. I didn't realise it was ready.

@tofumatt tofumatt self-requested a review August 13, 2018 10:23
@tofumatt tofumatt removed the Needs Copy Review Needs review of user-facing copy (language, phrasing) label Aug 13, 2018
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

Thanks for the changes and sorry for all the back-and-forth on this. I dig it! 👍

@sarahmonster
Copy link
Member

Thanks for all your patience with this, @nosolosw! This looks good by me now. 🚢 !

@oandregal oandregal merged commit 28e00cd into master Aug 13, 2018
@oandregal oandregal deleted the add/tags-to-prepublishpanel branch August 13, 2018 14:55
@mtias mtias added this to the 3.6 milestone Aug 16, 2018
return {
areTagsFetched: tagsTaxonomy !== undefined,
isPostTypeSupported: tagsTaxonomy && tagsTaxonomy.types.some( ( type ) => type === postType ),
hasTags: tagsTaxonomy && select( 'core/editor' ).getEditedPostAttribute( tagsTaxonomy.rest_base ).length,
Copy link
Member Author

Choose a reason for hiding this comment

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

I learned that pages don't have taxonomies the hard way #9082

onUpdatePostFormat={ onUpdatePostFormat }
suggestedPostFormat={ suggestion.id }
suggestionText={ sprintf(
__( 'Apply the "%1$s" format.' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

This misses a translators comment. Will try to address it in #9363

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.

9 participants