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

Blocks: Tag Cloud #7875

Merged
merged 19 commits into from Feb 15, 2019

Conversation

@jahvi
Copy link
Contributor

jahvi commented Jul 10, 2018

Description

This is my attempt at implementing the tag cloud widget as a Gutenberg block, it's still a work in progress but I wanted to get some initial feedback while I'm at it.

Continued from #4994.
Fixes #1801.

How has this been tested?

Tested in the latest master branch.

Screenshots

motion

Types of changes

New feature: tag cloud block

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@jahvi jahvi force-pushed the jahvi:add/block-tag-cloud branch from d315be6 to ff31b5d Jul 10, 2018

@jahvi jahvi referenced this pull request Jul 10, 2018

Closed

Blocks: Tag Cloud #4994

3 of 3 tasks complete

@sarahmonster sarahmonster added this to New blocks to review in The Great Block Audit of 2018 Jul 31, 2018

@jahvi jahvi force-pushed the jahvi:add/block-tag-cloud branch from ff31b5d to 4074ac6 Aug 16, 2018

@jahvi

This comment has been minimized.

Copy link
Contributor Author

jahvi commented Aug 16, 2018

Updated PR to match changes from #8287

@jahvi jahvi force-pushed the jahvi:add/block-tag-cloud branch from 4074ac6 to 76f5113 Aug 16, 2018

@jahvi jahvi force-pushed the jahvi:add/block-tag-cloud branch from 5f0f65e to 7b8cc64 Oct 1, 2018

@jahvi

This comment has been minimized.

Copy link
Contributor Author

jahvi commented Oct 1, 2018

Any update on this one? I rebased the latest changes from master but not sure if I should refactor anything else.

@Soean

This comment has been minimized.

Copy link
Member

Soean commented Oct 9, 2018

@jahvi Can you rebase it again? I will test the block on my Gutenberg installation.

@jahvi jahvi force-pushed the jahvi:add/block-tag-cloud branch from 7b8cc64 to 9d22461 Oct 9, 2018

@jahvi

This comment has been minimized.

Copy link
Contributor Author

jahvi commented Oct 9, 2018

@Soean Sure thing, I just rebased it now.

@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Oct 11, 2018

As we focus on phase 1 this will be moving to potential phase 2 and we can then consider it from there, For now, closing and moving to potential blocks for the future.

@karmatosed karmatosed closed this Oct 11, 2018

The Great Block Audit of 2018 automation moved this from New blocks to review to Done Oct 11, 2018

@karmatosed karmatosed reopened this Dec 14, 2018

@melchoyce melchoyce added this to In Progress in Porting widgets to blocks Dec 14, 2018

@jahvi jahvi force-pushed the jahvi:add/block-tag-cloud branch from 0ba24a6 to 7ffa162 Dec 15, 2018

@jahvi

This comment has been minimized.

Copy link
Contributor Author

jahvi commented Dec 15, 2018

Thanks for reopening, I just rebased and fixed some code to match the latest Gutenberg changes. I'm not sure how to fix the CI issues, can you guys advise?

@Soean

This comment has been minimized.

Copy link
Member

Soean commented Dec 15, 2018

Nice work! You have to delete the generated test files and regenerate them.

Delete:

  • core__tag-cloud__showTagCounts.json
  • core__tag-cloud__showTagCounts.parsed.json
  • core__tag-cloud__showTagCounts.serialized.html
  • core__tag-cloud.json
  • core__tag-cloud.parsed.json
  • core__tag-cloud.serialized.html

Run: GENERATE_MISSING_FIXTURES=y npm run test-unit test/integration/full-content/full-content.spec.js

@jahvi

This comment has been minimized.

Copy link
Contributor Author

jahvi commented Feb 9, 2019

@mapk I see, based on the gif looks like you're selecting the same taxonomy which won't trigger the onChange event indeed.

Should I make it so it goes back even when selecting the same taxonomy? Or should I keep the edit icon in the toolbar even when the placeholder is visible so it can be clicked to go back?

I tried to keep it consistent with the RSS block where the edit icon is hidden when the placeholder is visible.

@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Feb 11, 2019

Thanks for your work on this, @jahvi !!

I tried to keep it consistent with the RSS block where the edit icon is hidden when the placeholder is visible.

The reason the RSS block works this way is because there's an actual button in the setup screen that can take you back. In the case of the tag cloud, there isn't.

Should I make it so it goes back even when selecting the same taxonomy? Or should I keep the edit icon in the toolbar even when the placeholder is visible so it can be clicked to go back?

I don't think keeping the Edit icon there to click again for a preview is intuitive. BUT I really like the Edit icon to take me to the setup screen where I can select between "tags" or "categories".

So we've got a few options here:

  1. Include a "Preview" button next to the Edit icon similarly to the HTML block.

html-block

  1. Or add a button to the setup screen so that when they make their selection, they also need to click a "submit" button to bring them to the Preview mode.

How do either of those sound?

@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Feb 11, 2019

I guess the third option here could be to default the block to "tags" and just get rid of the setup screen completely. Only allow the "tag" or "category" option in the Inspector kind of how you have that there now.

@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented Feb 13, 2019

I guess the third option here could be to default the block to "tags" and just get rid of the setup screen completely. Only allow the "tag" or "category" option in the Inspector kind of how you have that there now.

Personally I think this makes the most sense, see #7875 (review).

Thanks for addressing my feedback. The code looks great to me! 🙂

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 14, 2019

Can we clarify what's remaining here? Can we get this in a shippable state for tomorrow? Happy to help with anything.

Remove extra parameters in taxonomies query
Co-Authored-By: jahvi <javiervd@gmail.com>

@jahvi jahvi requested review from notnownikki and talldan as code owners Feb 14, 2019

@jahvi

This comment has been minimized.

Copy link
Contributor Author

jahvi commented Feb 14, 2019

Should I go for the third option then? Remove the setup view and just allow for changing taxonomies on the sidebar?

It's a simple change so I can have it ready later today.

@melchoyce

This comment has been minimized.

Copy link
Contributor

melchoyce commented Feb 14, 2019

Yeah, let's do that. Thanks!

@jahvi

This comment has been minimized.

Copy link
Contributor Author

jahvi commented Feb 14, 2019

Removed the placeholder view now, I also changed the text when no tags are found to "No terms to show" since the previous "No taxonomies to show" didn't make much sense, hope that's ok.

motion

@mapk

mapk approved these changes Feb 14, 2019

Copy link
Contributor

mapk left a comment

This looks great! I think we can :shipit:

Aside as another PR to improve this:
One struggle I had was when I would select the block, many times I'd click on a link accidentally and the page would either reload to that link destination, or alert me that I'm leaving. I noticed that on the Latest Posts block, if I accidentally clicked a link, it would open in a new tab instead of reloading my current view. This worked better because I really just wanted to select the block. Maybe we can adopt that strategy for this in another PR?

Feedback was addressed as far as I can see

@gziolo

gziolo approved these changes Feb 15, 2019

Copy link
Member

gziolo left a comment

Awesome work @jahvi 💯

Code wise this PR is ready to land. When testing I noticed some very subtle visual differences between this block and the existing widget:

Block with tags selected:
screen shot 2019-02-15 at 09 31 21

Widget with tags selected:
screen shot 2019-02-15 at 09 32 06

Block with categories selected:
screen shot 2019-02-15 at 09 26 56

Widget with categories selected:
screen shot 2019-02-15 at 09 27 01

I have no idea if this is something that needs to be fixed or is fine to proceed as is. I'll leave the final word to @melchoyce and @mapk.

There is also third option available to Tag Cloud widget which I can't see on the block. Not sure what Link Categories is in the first place but it would be worth investigating.

screen shot 2019-02-15 at 09 34 36

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 15, 2019

One more thing which I want to confirm. Tag cloud widget is using wrapping div with tagcloud class name. Block uses wrapping p without this class. Is that expected? I personally don't see it as an issue, but maybe we want to stay closer to widgets in terms of syntax. This is how widgets look like:

screen shot 2019-02-15 at 10 45 23

@melchoyce

This comment has been minimized.

Copy link
Contributor

melchoyce commented Feb 15, 2019

Is the difference just the underlines? We should keep those.

There is also third option available to Tag Cloud widget which I can't see on the block. Not sure what Link Categories is in the first place but it would be worth investigating.

The site you're using probably is old enough that it had the "Links" section in wp-admin (or maybe we include it in development versions?). Links were hidden from all new sites in 3.5. We should conditionally support that, but it's lower priority IMO since it's a deprecated feature. We can make a new issue/PR for that later.

@melchoyce

This comment has been minimized.

Copy link
Contributor

melchoyce commented Feb 15, 2019

Can we also make the error messages change based on which taxonomy is chosen?

No categories to show.
No tags to show.

Or even:

Your site doesn't have any categories to show.
Your site doesn't have any tags to show.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 15, 2019

I tested this and I feel it's good enough to land to include in Gutenberg 5.1. Follow-up improvements to styling, and messages can happen separately.

@youknowriad youknowriad merged commit b58fed3 into WordPress:master Feb 15, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 15, 2019

Thanks everyone for the collaboration and @jahvi Awesome work

@melchoyce melchoyce moved this from In Progress to Done in Porting widgets to blocks Feb 15, 2019

@jahvi

This comment has been minimized.

Copy link
Contributor Author

jahvi commented Feb 15, 2019

Thanks all for your help, I'll be glad to keep helping improve it.

@jahvi jahvi deleted the jahvi:add/block-tag-cloud branch Feb 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment