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

Block Library: Add features to the Post Tags block. #20418

Closed
wants to merge 1 commit into from

Conversation

epiqueras
Copy link
Contributor

Description

This PR adds all the features of the tags theme function, https://codex.wordpress.org/Function_Reference/the_tags, to the Post Tags block as @mtias suggested in #19580 (comment).

How has this been tested?

It was verified that changing the before and after texts, and the separator, works as expected.

Screenshots

gif

Types of Changes

New Feature: The Post Tags block now allows you to edit the before and after texts, and the separator.

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.

@github-actions
Copy link

Size Change: +140 B (0%)

Total Size: 865 kB

Filename Size Change
build/block-library/index.js 116 kB +140 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 9.78 kB 0 B
build/block-editor/style.css 9.77 kB 0 B
build/block-library/editor-rtl.css 7.67 kB 0 B
build/block-library/editor.css 7.67 kB 0 B
build/block-library/style-rtl.css 7.49 kB 0 B
build/block-library/style.css 7.49 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.6 kB 0 B
build/components/index.js 190 kB 0 B
build/components/style-rtl.css 16.1 kB 0 B
build/components/style.css 16 kB 0 B
build/compose/index.js 5.76 kB 0 B
build/core-data/index.js 10.5 kB 0 B
build/data-controls/index.js 1.04 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 771 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 90.8 kB 0 B
build/edit-post/style-rtl.css 8.7 kB 0 B
build/edit-post/style.css 8.69 kB 0 B
build/edit-site/index.js 4.59 kB 0 B
build/edit-site/style-rtl.css 2.77 kB 0 B
build/edit-site/style.css 2.76 kB 0 B
build/edit-widgets/index.js 4.36 kB 0 B
build/edit-widgets/style-rtl.css 2.8 kB 0 B
build/edit-widgets/style.css 2.79 kB 0 B
build/editor/editor-styles-rtl.css 327 B 0 B
build/editor/editor-styles.css 328 B 0 B
build/editor/index.js 45.1 kB 0 B
build/editor/style-rtl.css 4.13 kB 0 B
build/editor/style.css 4.11 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.6 kB 0 B
build/format-library/style-rtl.css 500 B 0 B
build/format-library/style.css 501 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.45 kB 0 B
build/is-shallow-equal/index.js 711 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 215 B 0 B
build/list-reusable-blocks/style.css 216 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.02 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 878 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.54 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

$output .= '<a href="' . get_tag_link( $tag->term_id ) . '">' . $tag->name . '</a>' . ' | ';
}
return trim( $output, ' | ' );
$output = array_map(
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it'd be feasible to use the_tags() for rendering and just map the attributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That echos the output, and we lose the ability to add more features in the future.

@mapk mapk added the Needs Design Feedback Needs general design feedback. label Feb 25, 2020
@mapk
Copy link
Contributor

mapk commented Feb 25, 2020

Wow, a Post Tags block! Nice work! I tested it out and have a bit of feedback.

1. Using the keyboard to navigate was difficult.
The arrow keys didn't operate as i imagined they would. And highlighting the before and after text to type something else was a bit awkward. It would be great to refine the keyboard experience here.

keyboard 2020-02-24 22_19_09

2. I enjoy editing the dividers in place.
Editing the the dividers is definitely not obvious, but a fun little easter egg. I wonder if we can help share what can be done here in the description of the block?

3. The Before/After text was confusing.
I wasn't sure if this text was editable or not. I couldn't highlight the text to type something else in its place. When clicking into those sections, maybe hide the placeholder text similarly to how it works in a Paragraph block.

Screen Shot 2020-02-24 at 9 58 07 PM

I wonder if removing the "After text." might be a good idea so that it helps diminish the cognitive discernment between what is editable and what's not.

Maybe the "Before text." can be reworded to just say "Tags:" from the beginning? "Before text." appears to be more of a mistake here than any sort of help IMO.

Screen Shot 2020-02-24 at 10 32 08 PM

4. No tags.
Can we be a bit more descriptive here? Maybe, "No tags have been added to this post." I also think this text should match the grey of the placeholder text.

Screen Shot 2020-02-24 at 9 57 24 PM

5. Hash tags.
I'd like to be able to add hashtags to the front of each tag. Maybe this should exist in the sidebar settings?

Screen Shot 2020-02-24 at 10 01 57 PM

When I tried adding hashtags, I couldn't get it appended to the first tag.

Screen Shot 2020-02-24 at 10 33 38 PM

@mtias
Copy link
Member

mtias commented Feb 25, 2020

I'd like to be able to add hashtags to the front of each tag

I thought the same.

@epiqueras
Copy link
Contributor Author

  1. I enjoy editing the dividers in place.

Can we surface them through a particular background color or something?

  1. The Before/After text was confusing.

What's a better placeholder? Just ... maybe?

  1. Hash tags.

Should we support arbitrary prefixes, or is this limited enough in scope that we can make it a toggle/select thing?

@kjellr
Copy link
Contributor

kjellr commented Feb 25, 2020

Should the default separator here be a comma instead of a pipe? I feel like that tends to be used more commonly.

@shaunandrews
Copy link
Contributor

Maybe the "Before text." can be reworded to just say "Tags:" from the beginning?

Agreed. "Tags" would make more sense and encourage interaction with the text.

removing the "After text."

I don't think the $after value is very relevant for the block. On the theme function, the $after value allows you to close any html tags. I'm not sure there's much value in having any actual content after the tags and it feels like it adds unnecessary complexity to the block.

Should the default separator here be a comma

A big +1 here.

@epiqueras
Copy link
Contributor Author

Agreed. "Tags" would make more sense and encourage interaction with the text.

Agreed

I don't think the $after value is very relevant for the block. On the theme function, the $after value allows you to close any html tags. I'm not sure there's much value in having any actual content after the tags and it feels like it adds unnecessary complexity to the block.

Agreed, I was confused as to why it existed, to be honest.

A big +1 here.

Agreed

@epiqueras
Copy link
Contributor Author

@mtias Do you think these changes make sense? Do you see a use for the "after text"?

@carolinan carolinan mentioned this pull request May 19, 2020
6 tasks
@carolinan carolinan added the [Block] Post Tags Affects the Post Tags Block label Jun 4, 2020
@gziolo gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Oct 28, 2020
@gziolo
Copy link
Member

gziolo commented Oct 28, 2020

@mtias, can you check this PR and let us know if this is something that should be continued or can be closed. It's outdated now.

Base automatically changed from master to trunk March 1, 2021 15:43
@annezazu
Copy link
Contributor

Closing this out due to lack of feedback.

@annezazu annezazu closed this Jul 27, 2022
@gziolo gziolo deleted the add/post-tags-block-features branch July 28, 2022 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Post Tags Affects the Post Tags Block Needs Design Feedback Needs general design feedback. [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants