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

Quadrat: Add Join pattern #3745

Merged
merged 5 commits into from
May 6, 2021
Merged

Quadrat: Add Join pattern #3745

merged 5 commits into from
May 6, 2021

Conversation

scruffian
Copy link
Member

Changes proposed in this Pull Request:

Adds a "Join" pattern.

To test you need Jetpack installed and running on a custom domain, to make the subscription block work. It might be easier to test on a simple site.

Screenshot 2021-05-04 at 14 09 25

Note: The button / input on the subscription form doesn't match the theme. This is because of this issue:Automattic/jetpack#17590

It would also be really good to get this done so we don't need the custom CSS: WordPress/gutenberg#31459

Related issue(s):

#3642

@scruffian scruffian added this to the Quadrat v1 milestone May 4, 2021
@scruffian scruffian requested a review from a team May 4, 2021 13:11
@scruffian scruffian self-assigned this May 4, 2021
@scruffian scruffian changed the base branch from trunk to make/quadrat May 4, 2021 13:12
}
}
endif;

add_action( 'after_setup_theme', 'quadrat_register_block_patterns', 12 );
add_action( 'init', 'quadrat_register_block_patterns' );
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is needed so that the blocks are registered when we register the patterns. The problem is it moves Quadrat to the bottom of the patterns list drop down.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I see. I wonder if we can re-order the list somehow?

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.

This makes sense and looks good considering the upstream changes that should happen.

My only comment is maybe the pattern should be alignwide by default?

quadrat/inc/block-patterns.php Outdated Show resolved Hide resolved
}
}
endif;

add_action( 'after_setup_theme', 'quadrat_register_block_patterns', 12 );
add_action( 'init', 'quadrat_register_block_patterns' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I see. I wonder if we can re-order the list somehow?

@scruffian scruffian force-pushed the add/join-pattern branch 4 times, most recently from 8f7c006 to f3be992 Compare May 6, 2021 10:18
@scruffian
Copy link
Member Author

We can get it higher in the list:
Screenshot 2021-05-06 at 11 57 28

@kjellr do you have a preference for where this should go?

@scruffian scruffian force-pushed the add/join-pattern branch from 9d1944c to 628f691 Compare May 6, 2021 11:23
Comment on lines 13 to 16

.subscription-column {
text-align: right;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be inside this folder?

Copy link
Contributor

@MaggieCabrera MaggieCabrera left a comment

Choose a reason for hiding this comment

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

This makes sense and looks good considering the upstream changes that should happen.

+1

@scruffian scruffian merged commit 3168d73 into make/quadrat May 6, 2021
@scruffian scruffian deleted the add/join-pattern branch May 6, 2021 13:53
@kjellr
Copy link
Contributor

kjellr commented May 6, 2021

@kjellr do you have a preference for where this should go?

Theme patterns generally show up at the top of the list, so if we can do that I think that's the best route. (I haven't tested this PR yet, so it's possible you're already doing that now).

@@ -28,15 +27,18 @@ function quadrat_register_block_patterns() {
'listen-to-the-podcast',
);

if ( class_exists( 'WP_Block_Type_Registry' ) && \WP_Block_Type_Registry::get_instance()->is_registered( 'jetpack/subscriptions' ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should update the wpcom-only patterns in Blank Canvas to use this method, rather than loading them in their own separate wpcom.php file.

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

Successfully merging this pull request may close these issues.

4 participants