Add Gutenberg Block for the [jobs] shortcode #1521

alexsanford · 2018-06-18T22:40:21Z

This PR adds Job Manager's first Gutenberg block!

Note: This is related to #1287 but this makes no changes to the shortcode itself. The block simply renders the shortcode into the post_content. In the future, we can enhance this by giving the block its own independent rendering.

The block is not yet feature-complete, but it's ready for review and feedback. Part of the purpose of this PR is to explore how to create blocks such as this in Job Manager (and other plugins) and to get the groundwork set up.

With that in mind, here are the things for which I'm looking for feedback:

The Block Itself!

Of course, I want feedback on the block. Load it up in Gutenberg, and see how it feels. It's called "Jobs" in the block selector. Some notes to consider:

I tried to make it look and feel pretty similar to the frontend element rendered by the shortcode.
I don't like how things jump around on select. This was in an effort to make the unselected UI look like it's going to look on the frontend while also providing adequate inline controls. Maybe there's a way to do this that isn't quite as jarring?
Note that the jobs themselves at the bottom are just placeholders. There's some extra eyecandy that we could add in the future (having actual job info, showing pagination links at the bottom to reflect our pagination setting, etc) but none of it is necessary for this initial version.
The Categories UI is not finished. Its complexity will be similar to the Job Types UI, and I didn't want to dive in too deeply before getting some feedback.

The Block Code

I put the code in assets/blocks/jobs. It is not in the assets/js directory because I wanted it to be separate, and technically the block also contains CSS.
I split up some components, as you can see in the directory. For the most part, I'm importing the functionality through ES6 imports.
I am also using some WP hooks in the JS code. This is to make the block extensible by other plugins. When we actually extend it, we will probably need to make some changes (e.g. there is no way currently for an extension to add components to the edit UI)

The Build

The build is pretty standard webpack/babel, with some things borrowed from the Gutenberg build.
I've included it in our grunt-based existing build system.
When we start building the other blocks, there are a few things we may need to consider:
- Sharing components between blocks.
- Splitting bundles (for both synchronous and asynchronous loading).

Known Issues

Sometimes the withAPIData doesn't seem to work properly, and the Job Types UI gets stuck on "Loading". If this happens, just refresh the page, and it will probably work next time.
There are some issues that exist with the shortcode itself, including:
- When filters are shown, the text in the Keywords and Location boxes don't persist in the frontend.
- Sometimes the selected job types don't persist in the frontend as well.
The Job Types UI (and eventually the Categories UI) should not be displayed if the respective features are not enabled in the Settings.
The UI design could use some work. Currently it "jumps" on select because of things being shown/hidden. Maybe there is a way to move more things to the sidebar, or to "Quick Toolbar" at the top of the block.

To-do

Finish the Categories UI (waiting for some feedback first).
Add tests (unit and e2e).
Add linting (I think it would be great to have a linter running in CI for all of our block code, and this is a good time to set it up since we don't have any legacy code here yet 🙂 ).

dbtlr

Overall this is really great. I don't have a dev env to test this on at the moment, so my comments are mostly linting type comments. P.S. having a linter set up before you write any code is great for this exact reason.

Do we have a linting standard you suggest following? Is there one for Gutenberg already? I'm happy to get that setup here if you can link to the one most people are using. :)

dbtlr · 2018-06-19T18:02:02Z

Gruntfile.js

@@ -274,6 +282,9 @@ module.exports = function( grunt ){

 	grunt.registerTask( 'build-mixtape', [ 'shell:buildMixtape' ] );

+	grunt.registerTask( 'build-blocks', [ 'shell:webpack' ] );
+	grunt.registerTask( 'build-blocks:dev', [ 'shell:webpackDev' ] );
+
 	grunt.registerTask( 'build', [ 'gitinfo', 'clean', 'check-mixtape', 'check-mixtape-fatal', 'test', 'copy' ] );


Shouldn't build-blocks be added to this list?

Good catch! Done.

dbtlr · 2018-06-19T18:05:29Z

assets/blocks/jobs/edit.jsx

+/**
+ * Internal Dependencies.
+ */
+import Sidebar from './sidebar.jsx';


Most style guides have you put imports before const declarations, maybe this isn't the Gutenberg way, but it might be nice to flip this to stay consistent with other es6 best practices out there

Done. In Gutenberg itself the WordPress Dependencies are actually import's, but in this case I think it makes more sense to use assignment since they're already available in a global. No need to add extra stuff to our build.

But yes, since it's an assignment instead of an import, I agree that it should be under the other import's 🙂

dbtlr · 2018-06-19T18:07:22Z

assets/blocks/jobs/job-placeholder-list.jsx

+ * JobPlaceholder component.
+ */
+export default function JobPlaceholderList( { number, className } ) {
+	let placeholders = [];


const is generally preferable over let when possible, which in this case it is :)

Good catch. The linter will help a lot for these issues 🙂

dbtlr · 2018-06-19T18:08:40Z

assets/blocks/jobs/index.jsx

+	edit,
+
+	save: function( { attributes } ) {
+		let shortcodeParams = {};


It is generally more readable to use const here and the assign a new variable for the filter down below.

dbtlr · 2018-06-19T18:09:03Z

assets/blocks/jobs/sidebar.jsx

+ * @return {Object} The new shortcode parameters.
+ */
+function getShortcodeParameters( shortcodeParams, attributes ) {
+	let shortcodeParamNames = [


dbtlr · 2018-06-19T18:14:01Z

assets/blocks/jobs/types.jsx

+	try {
+		obj = JSON.parse( value );
+	} catch ( error ) {
+		// On error, just use the empty object.


What is the result of value failing here? Does this really just result in return null? If so it may be better to more explicitly do that here.

Yeah, it results in return null. I like that idea. Done.

dbtlr · 2018-06-19T18:15:14Z

assets/blocks/jobs/types.jsx

+	let keysToInclude = _.pickBy( obj, ( val ) => val );
+
+	// Create a comma-separated string.
+	value = Object.keys( keysToInclude ).join( ',' );


Re-assigning arguments is generally frowned upon as it can have other side-effects in certain circumstances. It is generally better to avoid that all together (most linting standards will pick up on this).

Yeah, good catch. Done.

dbtlr · 2018-06-19T18:16:27Z

assets/blocks/jobs/types.jsx

+
+	if ( attributes.showJobTypeFilters ) {
+		if ( jobTypes ) {
+			shortcodeParams.selected_job_types = jobTypes;


Same re-assign issue as above. The problem here is that this will re-assign the object properties in the original scope, aka, side-effects!!

donnapep · 2018-06-19T19:00:49Z

Tried to give this a test, but I don't have Gutenberg installed yet so the register_block_type function does not exist. Probably we shouldn't register any scripts either in that case:

alexsanford · 2018-06-20T11:08:52Z

Do we have a linting standard you suggest following? Is there one for Gutenberg already? I'm happy to get that setup here if you can link to the one most people are using. :)

@dbtlr Yeah, Gutenberg uses ESLint. I started taking a look into getting it set up, but I think it might take some time going through and understanding their setup. If you want to pick up where I left off, here's the branch I've been working on (note that it branches off of this one). Check out the Gutenberg repo for their linting config. The relevant files are package.json (for installing/running eslint), .eslintignore, .eslintrc.js, and eslint/config.js.

alexsanford · 2018-06-20T11:16:19Z

Tried to give this a test, but I don't have Gutenberg installed yet

Well, why not???

Good catch. I added a check for function_exists( 'register_block_type' ). I'm assuming that's the best way to do it. I haven't seen any documentation specifically stating how to check for it.

ice9js

I had an initial look and so far I found a few things that may need attention.
On a side note, should we consider using propTypes ? I think it could make things just a bit more obvious, especially when we're using HOCs.

ice9js · 2018-06-27T11:29:29Z

includes/class-wp-job-manager-blocks.php

+	}
+
+	private function __construct() {
+		if ( ! function_exists( 'register_block_type' ) ) {


We should move that into register_blocks to make sure Gutenberg has a chance to load before this check.

ice9js · 2018-06-27T11:38:41Z

assets/blocks/jobs/types.jsx

+	// Create a comma-separated string.
+	let strValue = Object.keys( keysToInclude ).join( ',' );
+
+	if ( ! strValue ) {


I wonder if this is really necessary as I see the same comparison here.
I guess we could return the empty string just as well and save ourselves a bit on complexity ?

ice9js · 2018-06-27T17:11:05Z

assets/blocks/jobs/types.jsx

+		if ( ! this.state.haveAPIData ) {
+			const { types } = this.props;
+
+			if ( types && ! types.isLoading && 'undefined' !== typeof types.data ) {


I noticed we never actually dispatch the request/call types.get() and so this function seems to always return false.

spencerfinnell · 2018-06-27T17:32:25Z

There's some extra eyecandy that we could add in the future (having actual job info, showing pagination links at the bottom to reflect our pagination setting, etc) but none of it is necessary for this initial version.

@alexsanford I think this is going to confuse people. It's not using any of the same markup as the current PHP templates, and any non-default theme will show something completely different.

I wrote an article about a lot of the pitfalls theme authors and users will find when trying to implement an existing shortcode as a Gutenberg block here: https://blog.bigboxwc.com/gutenberg-and-themes/

To me this fits perfectly with my examples and a live preview of the existing [jobs] shortcode with controls to toggle current shortcode attributes would be more beneficial. Not discounting the effort put in to the block just not sure how much users would gain by seeing a block that will likely not reflect the true output.

ice9js · 2018-06-28T14:41:50Z

wp-job-manager.php

@@ -74,6 +74,7 @@ public function __construct() {
 		include_once( JOB_MANAGER_PLUGIN_DIR . '/includes/class-wp-job-manager-geocode.php' );
 		include_once( JOB_MANAGER_PLUGIN_DIR . '/includes/class-wp-job-manager-blocks.php' );
 		include_once( JOB_MANAGER_PLUGIN_DIR . '/includes/class-wp-job-manager-cache-helper.php' );
+		include_once( JOB_MANAGER_PLUGIN_DIR . '/includes/class-wp-job-manager-blocks.php' );


Just noticed this line is duplicated above :)

alexsanford · 2018-06-29T10:47:16Z

@spencerfinnell Thanks for your comment! I really appreciate you bringing this up. I've been doing a lot of thinking about this, and discussing it with the team.

I'm starting to agree. For the sake of creating blocks that directly replace the shortcodes, probably rendering server-side is the way to go, as you've outlined in your post. I'm going to explore that and see how well it will work in Job Manager. I suspect we may run into some issues with [jobs] in particular since it's so complex, but it may work out pretty well for the simpler shortcodes.

My original intent with this PR was to create a block that would initially output the shortcode, and eventually move to rendering in a more pure-Gutenberg way. I'm realizing that probably the pure-Gutenberg solution is not an evolution of this shortcode-based block, but rather something we will have to create separately from the ground up.

I'm envisioning blocks or pages that lay out their UI as a collection of child blocks, determined by a block template. Themes will then be able to modify that template in a way that Gutenberg can understand, so the same customizations can be shown in the editor and on the frontend. This is also something that site admins could potentially do, without having to modify any theme code. This should go a long way toward solving the problems you've discussed here.

For example, maybe the [jobs] shortcode is replaced by a set of blocks that can simply be added to a page. There would be a block for each of the different pieces: the search boxes, the filters for Types and Categories, the actual Job Listings, etc. There would then be a page template (a Gutenberg template, not a traditional PHP template) that would set this up with sensible defaults, and the theme could modify that template (or provide alternatives).

A simpler example is the [job] shortcode. This could become a block with child blocks for the title, company info, description, etc. Again, these children and their layout can be set up by a block template which the theme or admin can modify.

Note that this is all still pretty speculative, since much of this doesn't exist yet in Gutenberg. But I'm pretty confident that this idea fits well with the direction that WordPress is moving, especially with the Customizer changes that are coming.

cc @jom @donnapep @dbtlr

jom · 2018-11-27T16:10:05Z

Closing (for now) in favor of #1545

alexsanford added this to the 1.32.0 milestone Jun 18, 2018

alexsanford self-assigned this Jun 18, 2018

alexsanford requested review from jom, donnapep and ice9js June 18, 2018 22:40

dbtlr suggested changes Jun 19, 2018

View reviewed changes

dbtlr force-pushed the add/jobs-shortcode-block branch from f66fd9f to 1272ff7 Compare June 27, 2018 15:24

dbtlr mentioned this pull request Jun 27, 2018

Add Webpack/Eslinting for Gutenberg blocks #1533

Merged

alexsanford added 9 commits June 27, 2018 17:41

Add block files

0ce4c5d

Enqueue block JS

cdddada

Move const declarations below imports

9bddbb2

Use const when possible

95e207d

Explicit return on error in transformJobTypeValue

13f1141

Remove parameter manipulation with side-effects

d63d6ce

Fix: hiding filters in editor when unselected

8cb90da

Filter Types when all filters are hidden

2bc9f63

Update the build

56775c4

ice9js requested changes Jun 27, 2018

View reviewed changes

Fix block registration

cb00ae7

alexsanford force-pushed the add/jobs-shortcode-block branch from 1272ff7 to cb00ae7 Compare June 27, 2018 17:21

ice9js reviewed Jun 28, 2018

View reviewed changes

alexsanford mentioned this pull request Jul 10, 2018

[WIP] Gutenberg block for the [jobs] shortcode using Server-Side Rendering #1545

Closed

alexsanford added 2 commits October 9, 2018 20:19

Add build JS

68b5777

Remove usage of withApiData

6430874

jom closed this Nov 27, 2018

jom removed this from the 1.32.0 milestone Dec 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Gutenberg Block for the [jobs] shortcode #1521

Add Gutenberg Block for the [jobs] shortcode #1521

alexsanford commented Jun 18, 2018 •

edited

dbtlr left a comment

dbtlr Jun 19, 2018

alexsanford Jun 20, 2018

dbtlr Jun 19, 2018

alexsanford Jun 20, 2018

dbtlr Jun 19, 2018

alexsanford Jun 20, 2018

dbtlr Jun 19, 2018

alexsanford Jun 20, 2018

dbtlr Jun 19, 2018

alexsanford Jun 20, 2018

dbtlr Jun 19, 2018

alexsanford Jun 20, 2018

dbtlr Jun 19, 2018

alexsanford Jun 20, 2018

dbtlr Jun 19, 2018

alexsanford Jun 20, 2018

donnapep commented Jun 19, 2018 •

edited

alexsanford commented Jun 20, 2018

alexsanford commented Jun 20, 2018

ice9js left a comment

ice9js Jun 27, 2018

ice9js Jun 27, 2018

ice9js Jun 27, 2018 •

edited

spencerfinnell commented Jun 27, 2018

ice9js Jun 28, 2018

alexsanford commented Jun 29, 2018

jom commented Nov 27, 2018

Add Gutenberg Block for the [jobs] shortcode #1521

Add Gutenberg Block for the [jobs] shortcode #1521

Conversation

alexsanford commented Jun 18, 2018 • edited

The Block Itself!

The Block Code

The Build

Known Issues

To-do

dbtlr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donnapep commented Jun 19, 2018 • edited

alexsanford commented Jun 20, 2018

alexsanford commented Jun 20, 2018

ice9js left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ice9js Jun 27, 2018 • edited

Choose a reason for hiding this comment

spencerfinnell commented Jun 27, 2018

Choose a reason for hiding this comment

alexsanford commented Jun 29, 2018

jom commented Nov 27, 2018

alexsanford commented Jun 18, 2018 •

edited

donnapep commented Jun 19, 2018 •

edited

ice9js Jun 27, 2018 •

edited