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

Add Gutenberg Block for the [jobs] shortcode #1521

Closed
wants to merge 12 commits into from

Conversation

alexsanford
Copy link
Contributor

@alexsanford alexsanford commented Jun 18, 2018

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 🙂 ).

@alexsanford alexsanford added this to the 1.32.0 milestone Jun 18, 2018
@alexsanford alexsanford self-assigned this Jun 18, 2018
Copy link
Contributor

@dbtlr dbtlr left a comment

Choose a reason for hiding this comment

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

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. :)

Gruntfile.js Outdated
@@ -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' ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Done.

/**
* Internal Dependencies.
*/
import Sidebar from './sidebar.jsx';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 🙂

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

edit,

save: function( { attributes } ) {
let shortcodeParams = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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

Choose a reason for hiding this comment

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

const!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

let keysToInclude = _.pickBy( obj, ( val ) => val );

// Create a comma-separated string.
value = Object.keys( keysToInclude ).join( ',' );
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good catch. Done.


if ( attributes.showJobTypeFilters ) {
if ( jobTypes ) {
shortcodeParams.selected_job_types = jobTypes;
Copy link
Contributor

Choose a reason for hiding this comment

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

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!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@donnapep
Copy link
Contributor

donnapep commented Jun 19, 2018

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:

screen shot 2018-06-19 at 2 54 59 pm

@alexsanford
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Well, why not??? :trollface:

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.

Copy link
Member

@ice9js ice9js left a comment

Choose a reason for hiding this comment

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

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.

}

private function __construct() {
if ( ! function_exists( 'register_block_type' ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

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

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

if ( ! strValue ) {
Copy link
Member

Choose a reason for hiding this comment

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

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 ?

if ( ! this.state.haveAPIData ) {
const { types } = this.props;

if ( types && ! types.isLoading && 'undefined' !== typeof types.data ) {
Copy link
Member

@ice9js ice9js Jun 27, 2018

Choose a reason for hiding this comment

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

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

@spencerfinnell
Copy link
Contributor

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.

@@ -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' );
Copy link
Member

Choose a reason for hiding this comment

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

Just noticed this line is duplicated above :)

@alexsanford
Copy link
Contributor Author

@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
Copy link
Member

jom commented Nov 27, 2018

Closing (for now) in favor of #1545

@jom jom closed this Nov 27, 2018
@jom jom removed this from the 1.32.0 milestone Dec 17, 2018
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.

6 participants