Blocks: Tag Cloud by jahvi · Pull Request #4994 · WordPress/gutenberg

jahvi · 2018-02-10T20:37:18Z

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.

Fixes #1801.

I tried to follow the "Categories" block for consistency but there are some areas I'm not sure about:

Right now I'm populating the select box with all taxonomies but I should just be getting only the ones registered with show_tagcloud as true, however I'm not sure how to do this, can this be done through the rest API?
To render the actual tag clous, should I "reimplement" the wp_generate_tag_cloud PHP function in my JS block or is there a better way to get the generated markup?

Any feedback would be appreciated! 😀

How Has This Been Tested?

Tested in the latest master branch.

Screenshots (jpeg or gifs if applicable):

Types of changes

New feature: tag cloud block

Checklist:

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

melchoyce · 2018-02-10T20:56:36Z

Hey @jahvi! Thanks for working on this.

What if the block had a placeholder when you first add it to your post?

jahvi · 2018-02-10T22:16:09Z

Thanks for the feedback @melchoyce that looks great! I'll work on that for the next update.

jahvi · 2018-02-11T14:00:24Z

I just improved the block a bit by adding a placeholder, alignment and server side rendering. Still tons left to do but it should be more usable now.

jahvi · 2018-02-17T16:07:43Z

Hey there, I just added some basic styles and managed to display the terms in the editor, I'm not very proud of the way I'm calculating the font sizes as it's almost a copy of how this is rendered on PHP but really open to suggestions.

aduth · 2018-02-19T18:41:03Z

blocks/library/tag-cloud/block.js

Minor: Lodash's _.map, which you're using, will gracefully handle undefined values:

n_ > _.map( undefined, () => '' ); []

aduth · 2018-02-19T18:43:33Z

blocks/library/tag-cloud/block.js

I can't help but think that taxonomyRestBase is redundant with taxonomySlug.

You might be able to eliminate it in favor of just the slug, taking advantage of the taxonomy helper included in the argument passed to withAPIData callback:

https://github.com/WordPress/gutenberg/tree/master/components/higher-order/with-api-data#usage

Thanks I wasn't aware of the taxonomy helper, it does simplify things a lot!

aduth · 2018-02-19T18:46:01Z

blocks/library/tag-cloud/block.js

Minor: It may be easier for React to reconcile† this in the render output if it were expressed as an empty value, e.g. null or undefined:

let terms; if ( termList.data ) { terms = ( <TermList terms={ termList.data } showTagCounts={ showTagCounts } className={ className } /> ); }

Or:

const terms = termList.data ? ( <TermList terms={ termList.data } showTagCounts={ showTagCounts } className={ className } /> ) : null;

† The assumption being that it shortcuts to avoid iterating over an empty value.

aduth · 2018-02-19T18:50:01Z

blocks/library/tag-cloud/editor.scss

These spaces should be tabs.

Use tabs, not spaces, to indent each property.

https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/#structure

aduth · 2018-02-19T18:51:44Z

blocks/library/tag-cloud/index.js

With #4069, align is now simpler to implement for a block via supports.

aduth · 2018-02-19T18:54:49Z

blocks/library/tag-cloud/index.php

I wouldn't expect align should work here, because you have no attribute schema defined on the server. See the Latest Posts block implementation as an example where attributes are to be defined on the server. These will automatically load into the client, so you don't need to duplicate there.

Eventually the idea is that all blocks will be defined this way : #2751

aduth · 2018-02-19T18:59:07Z

blocks/library/tag-cloud/term-list.js

You're mutating the original terms reference array here which, based on your usage of the return value termFontSizes below, I don't think is expected.

This might be a good candidate for an Array#map (returning an array of font sizes) or Array#reduce (returning an object of term name => font size).

// Calculate font size for each term return terms.map( ( term, i ) => { const countWeight = term.count - lowestCount; const fontSize = MIN + ( countWeight * fontStep ); return fontSize; } );

Thanks for pointing it out, I chose to use map since the terms order stays consistent I figured it would make the code simpler.

jahvi · 2018-02-19T22:00:36Z

Thanks for your feedback @aduth I just updated my PR.

westonruter · 2018-02-22T21:54:08Z

blocks/library/tag-cloud/block.js

We could avoid having to implement TermList which is a duplication of wp_tag_cloud() if we used ServerSideRender proposed in #780.

I agree, not sure what the status of that proposal is but it would have
been useful.

@jahvi the ServerSideRendercomponent has landed with #5495

Do you want to refactor your PR to make use of it?

Ah that's interesting, I'll refactor my PR then

Just updated the code to use the new ServerSideRender component, it does simplify things quite a lot 🙏🏽

jahvi · 2018-03-25T14:59:37Z

Just added this to make sure only taxonomies with show_cloud = true are displayed in the options, and added some some fixtures to make test pass.

tofumatt · 2018-07-05T22:57:04Z

It looks like this PR has stalled–I've linked it to the issue so we can use it as prior art, but because it hasn't been worked on in three months I'm going to close it.

@jahvi If that's my bad and you were waiting on someone to review something, please let us know and we can re-open this/keep iterating.

jahvi · 2018-07-07T21:39:21Z

Hey @tofumatt I just rebased the latest branches onto this to try to bring this PR back to life (not sure why my latest changes don't show, maybe because PR is closed)

I'm having an issue now where my use of the withAPIData HOC is giving me the following error:

react-dom.min.82e21c65.js:110 TypeError: Cannot read property 'getAllResponseHeaders' of undefined
    at getResponseHeaders (request.js:59)
    at Object.<anonymous> (request.js:87)
    at Object.<anonymous> (load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,underscore,backbone,moxiejs,plupload,jquery-ui-core,jquery-ui-widget,jquery-ui-mouse,jquery-ui-&load[]=sortable&ver=4.9.7:2)
    at i (load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,underscore,backbone,moxiejs,plupload,jquery-ui-core,jquery-ui-widget,jquery-ui-mouse,jquery-ui-&load[]=sortable&ver=4.9.7:2)
    at Object.add [as done] (load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,underscore,backbone,moxiejs,plupload,jquery-ui-core,jquery-ui-widget,jquery-ui-mouse,jquery-ui-&load[]=sortable&ver=4.9.7:2)
    at Array.<anonymous> (load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,underscore,backbone,moxiejs,plupload,jquery-ui-core,jquery-ui-widget,jquery-ui-mouse,jquery-ui-&load[]=sortable&ver=4.9.7:2)
    at Function.each (load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,underscore,backbone,moxiejs,plupload,jquery-ui-core,jquery-ui-widget,jquery-ui-mouse,jquery-ui-&load[]=sortable&ver=4.9.7:2)
    at Object.<anonymous> (load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,underscore,backbone,moxiejs,plupload,jquery-ui-core,jquery-ui-widget,jquery-ui-mouse,jquery-ui-&load[]=sortable&ver=4.9.7:2)
    at Function.a.Deferred (load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,underscore,backbone,moxiejs,plupload,jquery-ui-core,jquery-ui-widget,jquery-ui-mouse,jquery-ui-&load[]=sortable&ver=4.9.7:9)
    at Object.then (load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,underscore,backbone,moxiejs,plupload,jquery-ui-core,jquery-ui-widget,jquery-ui-mouse,jquery-ui-&load[]=sortable&ver=4.9.7:2)

From what I read this HOC is going to be deprecated but I thought it would be safe to use for now as other components are still using it, can anyone advice what the problem might be? I guess something changed in the HOC implementation that prevents this from working as it did before but comparing it with the other components I can't see any major differences.

jahvi · 2018-07-10T18:03:11Z

I managed to make the tag cloud block work with the new withSelect HOC so it should be ready for review, can this PR be opened again so I can push the new code or should I submit a new one?

tofumatt · 2018-07-10T18:07:44Z

Looks like the branch was force-pushed to so I can't reopen this PR:

If you open a new PR we can look at it, feel free to reference this one as well just to keep track of things 😄

jahvi · 2018-07-10T20:44:00Z

No problem, I just created a new PR #7875

jahvi mentioned this pull request Feb 10, 2018

Add Block: Tag Cloud #1801

Closed

jahvi force-pushed the add/block-tag-cloud branch from a223832 to a57ce61 Compare February 11, 2018 13:40

jahvi force-pushed the add/block-tag-cloud branch from 5f358e4 to c90cecb Compare February 17, 2018 09:07

aduth reviewed Feb 19, 2018

View reviewed changes

westonruter reviewed Feb 22, 2018

View reviewed changes

jahvi force-pushed the add/block-tag-cloud branch 2 times, most recently from 3ac4710 to 7ac38ae Compare March 4, 2018 15:53

jahvi force-pushed the add/block-tag-cloud branch from 7ac38ae to 9029500 Compare March 25, 2018 14:07

jahvi added 15 commits May 4, 2018 18:47

Initial implementation of tag cloud block

1ba51ec

Add placeholder when taxonomy is not set

b699ca3

Add server side rendering for tag cloud

51369e1

Add alignment functionality to tag cloud

a622a1e

Add initial styles for term list

622e90e

Change term font size depending on post count

8d2f035

Simplify term font size calculations

447c9f9

Fix PHP formatting warnings

265567a

Use helper to get taxonomy rest base

1688363

Swap alignment functionality to use "supports"

d36a5c1

Change indentation to tabs

e80a38e

Register attributes server side

3f7b2b1

Prevent mutation when calculating font sizes

fa4e306

Ignore taxonomies with "show_cloud" false

4876922

Add fixtures

c8a4bed

jahvi force-pushed the add/block-tag-cloud branch from fee27ae to c8a4bed Compare May 4, 2018 17:48

Refactor block to use ServerSideRender component

f4685bc

tofumatt closed this Jul 5, 2018

jahvi mentioned this pull request Jul 10, 2018

Blocks: Tag Cloud #7875

Merged

4 tasks

Conversation

jahvi commented Feb 10, 2018 • edited by tofumatt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

Uh oh!

melchoyce commented Feb 10, 2018

Uh oh!

jahvi commented Feb 10, 2018

Uh oh!

jahvi commented Feb 11, 2018

Uh oh!

jahvi commented Feb 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jahvi commented Feb 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jahvi commented Mar 25, 2018

Uh oh!

tofumatt commented Jul 5, 2018

Uh oh!

jahvi commented Jul 7, 2018

Uh oh!

jahvi commented Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tofumatt commented Jul 10, 2018

Uh oh!

jahvi commented Jul 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jahvi commented Feb 10, 2018 •

edited by tofumatt

Loading

jahvi commented Jul 10, 2018 •

edited

Loading