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

Try: A block-based version of Mayland #2802

Merged
merged 15 commits into from
Feb 26, 2021
Merged

Try: A block-based version of Mayland #2802

merged 15 commits into from
Feb 26, 2021

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Nov 11, 2020

This PR is the beginnings of a block-based version of Mayland.

Screenshots

Original Block-based
maylanddemo wordpress com_ dotcomthemes test__page_id=12
Original Block-based
maylanddemo wordpress com_about_ dotcomthemes test__page_id=2
Original Block-based
maylanddemo wordpress com_2019_09_05_overcoming-depth-of-field-limitations_ dotcomthemes test__p=1

@kjellr kjellr requested a review from a team November 11, 2020 16:45
@kjellr kjellr self-assigned this Nov 11, 2020
Copy link
Contributor

@jffng jffng left a comment

Choose a reason for hiding this comment

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

In general this LGTM.

I tested this on 9.3 (the latest plugin release as of this review) and the font families are not transformed nor applied.

As of 9.4.0rc1, the font families work but the template parts aren't loading in the site editor:

Screen Shot 2020-11-17 at 12 30 39 PM

@MaggieCabrera
Copy link
Contributor

I do see header and footer on 9.3.0 but not the fonts 😃

@kjellr
Copy link
Contributor Author

kjellr commented Nov 18, 2020

Yeah, I think the template parts issue is a 9.4 bug. Filed here.

@kjellr
Copy link
Contributor Author

kjellr commented Feb 1, 2021

This has been dormant for a while, so here's a fresh set of things that it needs:

  • Reformat theme.json to the new spec.
  • Remove theme slugs in templates.
  • Consider removing the spacing overrides in style.css.
  • Add query pagination.
  • Double check that comments are working.

cc @scruffian

@MaggieCabrera
Copy link
Contributor

I've worked a bit on this theme. One of the things I found was that when adding the Next/Previous links to a single post template I was not able to make the "Previous/Next post" text a link to the post just using the block. The final result of the block will require some extra CSS to make it fit the design of the theme as well (I haven't added it yet but I plan to do so unless there are any objections to that)

Original Block-Based
Screenshot 2021-02-15 at 14 58 07 Screenshot 2021-02-15 at 14 57 58

@scruffian
Copy link
Member

I feel like that's a pretty common pattern so should be added to the editor

@kjellr
Copy link
Contributor Author

kjellr commented Feb 15, 2021

Yeah, I agree — that should be a variation for the core block. If we do decide to build it in here, I suggest we do it as a block style maybe?

@MaggieCabrera
Copy link
Contributor

Yeah, I agree — that should be a variation for the core block. If we do decide to build it in here, I suggest we do it as a block style maybe?

I'm not sure if a block style is the best idea here since turning off the block style would make it look not so great. I'd say it's best if it's just styled by the theme (I'm talking in terms of just the font sizes and positioning of the links). Hopefully, the block will allow for more styling in the future too.

Besides, the issue with the "Previous/Next post" not being a link to the post too can't be solved by a block style either. I opened an issue for the block, which is also a very very new block to be using, so I suppose it will only improve with time (and our contributions, ofc): WordPress/gutenberg#29032

@kjellr
Copy link
Contributor Author

kjellr commented Feb 16, 2021

If it doesn't exist the way we need it to yet, I'd suggest we just leave that post pagination out of the theme and open a ticket to add it later.

@MaggieCabrera
Copy link
Contributor

I added the issue for the post pagination here and removed the block for the moment

@MaggieCabrera
Copy link
Contributor

MaggieCabrera commented Feb 16, 2021

For reference: this Gutenberg issue holds the discussion for implementing letter spacing controls on FSE

*/
function mayland_blocks_scripts() {
// Enqueue front-end styles.
wp_enqueue_style( 'mayland-blocks-styles', get_template_directory_uri() . '/style.css' );
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this line? I thought this file was enqueued by default. Also we could just use get_stylesheet_uri...

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently we do, if I remove it it doesn't load automatically


body {
-webkit-font-smoothing: antialiased;
-moz-osx-font-smoothing: grayscale;
Copy link
Member

Choose a reason for hiding this comment

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

This feels like it should be part of Gutenberg

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if every theme will want to have this on their stylesheet, feels like something we want for this specific font family.

What is under /* Set editor widths. */ would be something we'd be able to remove once the alignment PR lands most likely.
The rest of the styles (header spacing and letter spacing) are dependant on WordPress/gutenberg#23294 and WordPress/gutenberg#23228 respectively

Copy link
Member

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

This looks good. I think everything in style.css should be in Gutenberg somehow, so it would be good to have issues for it if there aren't already.

@MaggieCabrera
Copy link
Contributor

This is a list of the editor issues that are affecting this theme:

@danieldudzic danieldudzic self-requested a review February 26, 2021 15:09
Copy link
Contributor

@danieldudzic danieldudzic left a comment

Choose a reason for hiding this comment

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

I didn't find any blockers for the launch :)

Just a few issues with the frontend styling:

  1. Layout Grid columns are displaying one under another
    Screenshots: https://cldup.com/BwY5yx1W2S.png / https://cldup.com/cnI7FwSEtZ.png

  2. Button styling issue
    Screenshots: https://cldup.com/oHi6rbHij4.png / https://cldup.com/B7j0OfyOyZ.png

  3. Contact form missing styling: https://cldup.com/BZUulYROfK.png

@MaggieCabrera
Copy link
Contributor

I didn't find any blockers for the launch :)

Just a few issues with the frontend styling:

Looks like some of those are Jetpack blocks causing issues. I didn't see problems with the Button Block on my testing, though.

Screenshot 2021-02-26 at 17 06 44

These issues will be easier to test and report on once the theme is on wpcom.

@MaggieCabrera MaggieCabrera merged commit 0196fe5 into trunk Feb 26, 2021
@MaggieCabrera MaggieCabrera deleted the try/mayland-blocks branch February 26, 2021 16:39
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