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

Post Tags: Fix bug where no tags are rendered #24082

Merged
merged 14 commits into from
Aug 3, 2020
Merged

Post Tags: Fix bug where no tags are rendered #24082

merged 14 commits into from
Aug 3, 2020

Conversation

jeyip
Copy link
Contributor

@jeyip jeyip commented Jul 21, 2020

Description

This PR addresses a task in #21087 and is meant to add feature parity to FSE blocks. Previously, when the post tags block was added to a view, post tags were not being loaded. This PR implements a solution similar to what @Addison-Stavlo did for the post excerpt block in #23945.

Important Notes

  • From my understanding, tags can only be applied to posts, not pages.
    • While viewing a page in the site editor (ex. static home page), and not a post, it seems like there are no tags to display. (Please let me know if this assumption is incorrect)
    • In the situation where post tags blocks are used on a page, I render a warning that informs the user that the post tags block on a page doesn't make sense.
    • An alternative might be to prevent the user from even inserting post tags blocks on pages.
  • Adding a post tags block inside of a query loop block will add tags to all posts

How has this been tested?

Tested on local docker wp-env environment.

  1. Create a post and add tags in the settings sidebar
  2. Navigate to the site editor
  3. In the template dropdown, navigate to the recently created post
  4. Add the post tags block
  5. Verify that the correct tags are applied

Screenshots

Query Loop Index

post-tags-query-loop

Singular

post-tags-singular

Static Home Page (Rendering post tags on pages doesn't seem to make sense. Instead I render a warning)

post-tags-static-home-page

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.

@jeyip jeyip changed the title Fix/post tag rendering bug Post Tags: Fix bug where no tags are rendered Jul 21, 2020
@jeyip jeyip changed the title Post Tags: Fix bug where no tags are rendered Post Tags: Fix bug where no tags are rendered (In Progress) Jul 21, 2020
@ockham
Copy link
Contributor

ockham commented Jul 21, 2020

  • From my understanding, tags can only be applied to posts, not pages.
    • While viewing a page in the site editor (ex. static home page), and not a post, it seems like there are no tags to display. (Please let me know if this assumption is incorrect)

I think this is accurate -- with a caveat, plugins can override that behavior. But I think it's fine for a first iteration to only look at the post type; maybe a TODO comment that mentions that we should check whether the post_tag taxonomy is registered for the page post type. (We might be able to gather this information from the REST API, but I'm not even sure we really can.)

  • In the situation where post tags blocks are used on a page, I render a warning that informs the user that the post tags block on a page doesn't make sense.
  • An alternative might be to prevent the user from even inserting post tags blocks on pages.

I would find that preferable TBH -- UX-wise, I think it makes sense to hide something from the user that doesn't make sense in the given context, rather than informing them after the fact. I'm doing something similar here for the Post Content Block when used outside of templates and template parts: #24006

@ockham
Copy link
Contributor

ockham commented Jul 21, 2020

(We might be able to gather this information from the REST API, but I'm not even sure we really can.)

Oh, looks like we can: http://localhost:8888/index.php?rest_route=%2Fwp%2Fv2%2Ftaxonomies%2Fpost_tag%2F&context=edit&_locale=user has a types field that's set to [ 'post' ]. So we would probably need to check that array.

However, I'm not sure we can do that easily during Gutenberg's client-side block registration (i.e. in __experimentalRegisterExperimentalCoreBlocks) right now, since network fetches aren't easily available in that function. So let's keep this for a later iteration.

@ockham
Copy link
Contributor

ockham commented Jul 21, 2020

However, I'm not sure we can do that easily during Gutenberg's client-side block registration (i.e. in __experimentalRegisterExperimentalCoreBlocks) right now, since network fetches aren't easily available in that function. So let's keep this for a later iteration.

(Alternatively, we might do this server-side, once WordPress/wordpress-develop#419 gets merged.)

@ockham ockham mentioned this pull request Jul 21, 2020
6 tasks
@jeyip
Copy link
Contributor Author

jeyip commented Jul 21, 2020

I would find that preferable TBH -- UX-wise, I think it makes sense to hide something from the user that doesn't make sense in the given context, rather than informing them after the fact.

@ockham Your reasoning makes sense. Thanks for the insight into how to make this possible. 🙏 I'm happy to switch gears, but how do you feel about checking in with Jon before finalizing the decision?

cc @dubielzyk for opinions on how to handle the post tags block

@ockham
Copy link
Contributor

ockham commented Jul 22, 2020

However, I'm not sure we can do that easily during Gutenberg's client-side block registration (i.e. in __experimentalRegisterExperimentalCoreBlocks) right now, since network fetches aren't easily available in that function. So let's keep this for a later iteration.

(Alternatively, we might do this server-side, once WordPress/wordpress-develop#419 gets merged.)

Exploring an alternative that doesn't need WordPress/wordpress-develop#419 in #24114. If that flies, we can carry the principle over here.

@jeyip jeyip changed the title Post Tags: Fix bug where no tags are rendered (In Progress) Post Tags: Fix bug where no tags are rendered Jul 22, 2020
@ockham
Copy link
Contributor

ockham commented Jul 22, 2020

Exploring an alternative that doesn't need WordPress/wordpress-develop#419 in #24114. If that flies, we can carry the principle over here.

Didn't fly 😬 Ignore for now 😅

@dubielzyk
Copy link

cc @dubielzyk for opinions on how to handle the post tags block

I agree with @ockham that we ideally shouldn't allow them to add it to the post. Is it possible to hide it from the block inserter? If we show it, the warning is good, but I'd wanna refine the copy and perhaps add a little warning symbol there. It seems too "not wrong" at the moment.

@jeyip
Copy link
Contributor Author

jeyip commented Jul 22, 2020

Didn't fly 😬 Ignore for now 😅

Sad that it didn't work out, but A+ for effort 👏

I agree with @ockham that we ideally shouldn't allow them to add it to the post. Is it possible to hide it from the block inserter?

@ockham It sounds like the way forward might be to prevent post tags from being registered in __experimentalRegisterExperimentalCoreBlocks. Ideally we would evaluate post tags fetched from the API and conditionally register the post tags block like you've done here https://github.com/WordPress/gutenberg/pull/24006/files. Does that sound correct?

If we show it, the warning is good, but I'd wanna refine the copy and perhaps add a little warning symbol there.

@dubielzyk The warning symbol makes sense to me. Would it be okay to add this as an issue to the create design board and circle back? It seems like an easy change, but it would update all instances of warnings in the site editor and post editor. I think raising this with a broader audience would be nice to ensure we're not missing an important consideration.

@dubielzyk
Copy link

dubielzyk commented Jul 22, 2020

The warning symbol makes sense to me. Would it be okay to add this as an issue to the create design board and circle back? It seems like an easy change, but it would update all instances of warnings in the site editor and post editor. I think raising this with a broader audience, would be nice to make sure we're not missing anything.

So you'd be using the standard "block render error" that's used for all other blocks as well? If so, then yeah, let's just use that :) But ideally we would wanna prevent them to add it at all if possible.

@jeyip
Copy link
Contributor Author

jeyip commented Jul 23, 2020

Work is also being done on the post categories block by @Copons. There are discussions about similar UX decisions (hiding blocks vs. showing warnings)

Here is what we know:

  • The design preference is to hide blocks from the inserter instead of displaying warnings when tags are not supported.
  • We should keep the UX between post categories and post tags consistent.
  • Hiding blocks is probably outside of the scope of this PR and Jacopo's PR.

How would we feel about creating a separate issue to tackle hiding blocks? We could unblock the current PRs and I could follow up on the new issue immediately. I'd ensure that hiding both post tags and post categories is handled in a consistent way. @ockham

@ockham
Copy link
Contributor

ockham commented Jul 23, 2020

Work is also being done on the post categories block by @Copons. There are discussions about similar UX decisions (hiding blocks vs. showing warnings)

Here is what we know:

  • The design preference is to hide blocks from the inserter instead of displaying warnings.
  • We should keep the UX between post categories and post tags consistent.
  • Hiding blocks is probably outside of the scope of this PR and Jacopo's PR.

How would we feel about creating a separate issue to tackle hiding blocks? We could unblock the current PRs and I could follow up on the new issue immediately. I'd ensure that hiding both post tags and post categories is handled in a consistent way. @ockham

Sounds good 👍

@Copons
Copy link
Contributor

Copons commented Jul 30, 2020

This is working great for me!
I've left a few very minor suggestions, but this is basically ready!

Copy link
Contributor Author

@jeyip jeyip left a comment

Choose a reason for hiding this comment

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

This is working great for me!
I've left a few very minor suggestions, but this is basically ready!

@Copons Thanks for reviewing this! I think we're in the home stretch 🎉

Copy link
Member

@vindl vindl left a comment

Choose a reason for hiding this comment

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

This worked well for me. Nice work @jeyip! :shipit:

@vindl vindl added this to the Gutenberg 8.7 milestone Aug 3, 2020
@vindl vindl merged commit 9180ab5 into WordPress:master Aug 3, 2020
@jeyip jeyip deleted the fix/post-tag-rendering-bug branch August 3, 2020 20:40
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.

7 participants