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 some samples as Sensei patterns #5173

Merged
merged 4 commits into from
May 26, 2022

Conversation

renatho
Copy link
Contributor

@renatho renatho commented May 23, 2022

Fixes #5137

Changes proposed in this Pull Request

  • It registers some samples as Sensei patterns (copied from the patterns directory). There are 2 methods to register patterns for courses or lessons.
  • It also registers the Sensei category.

Testing instructions

  • Create a new course, navigate through the wizard, and make sure you see the registered patterns in the last step.
  • Also, check that you can see the patterns in the inserter filtering it by "Sensei LMS" category.
  • Repeat the test with a new lesson.

Known issues

  • Sometimes, I see some weird behaviors with the BlockPreview component, displaying it with the wrong height. If I activate the Gutenberg plugin, it works better. I think we could postpone our decision of what to do, at the moment that we create the final patterns to see if the issue will happen with them. If yes, I think we could set a maximum height to the thumbnails as a temporary workaround until the Gutenberg fix is in the versions we support.

Screenshot / Video

Screen Shot 2022-05-23 at 19 36 23

Screen Shot 2022-05-23 at 19 36 47

@renatho renatho added this to the Course and Lesson Wizard milestone May 23, 2022
@renatho renatho self-assigned this May 23, 2022
*
* @return string|false Post type on success, false on failure.
*/
private function get_early_post_type() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed this method in order to identify the post type in the init hook because the documentation says that it should be registered in this hook.

@renatho renatho requested a review from a team May 23, 2022 22:44
@renatho renatho marked this pull request as ready for review May 23, 2022 22:47
Copy link
Contributor

@aaronfc aaronfc left a comment

Choose a reason for hiding this comment

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

Looks promising but it is not working for me.

First of all, as a word of advise, I had some issues testing because I had a code snipped from your previous pull request which was overriding the new behaviour... took me a while to identify that haha. But I disabled it and it is still not working.

I am using WP 6.0 and at least for me the pattern category is not being correctly created. I have identified the issue being that the get_early_post_type returns false when the XHR request is being done from the frontend to get the patterns to wp-json/wp/v2/block-patterns/patterns?_locale=user.

I am going to test also in WP 5.9.
Edit: It is working fine in WP 5.9. Where the request to block-patterns/ is not seen in the network tab.

// Register block pattern category.
if ( in_array( $post_type, [ 'course', 'lesson' ], true ) ) {
register_block_pattern_category(
'sensei-lms',
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can extract this category to a constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to a constant in this commit: 7b9332b

@renatho
Copy link
Contributor Author

renatho commented May 25, 2022

I am using WP 6.0 and at least for me the pattern category is not being correctly created. I have identified the issue being that the get_early_post_type returns false when the XHR request is being done from the frontend to get the patterns to wp-json/wp/v2/block-patterns/patterns?_locale=user.

Hey @aaronfc! Good catch! I was thinking to return to the approach I started using - use the current_screen hook instead of the init hook, which the documentation recommends, but it seems it's not reliable.

@renatho
Copy link
Contributor Author

renatho commented May 25, 2022

Based on the last comments, I changed this PR to register the patterns regardless the post type, and I created another issue to work on that check since it's not very simple.

Copy link
Contributor

@aaronfc aaronfc left a comment

Choose a reason for hiding this comment

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

Not sure if this was ready for a new review but I tested and found some (new?) issues: (Tested both in WP 5.9.3 and WP 6.0).

Pattern disappears after closing the modal (courses)

Kapture.2022-05-26.at.09.09.36.mp4

Wizard is not working when creating Lessons

image

Edit: The last one (Wizard not working in Lessons) is happening also in feature/course-lesson-wizard. Reported here and looking into it.

@renatho
Copy link
Contributor Author

renatho commented May 26, 2022

@aaronfc, it was ready! I forgot to re-request the review.

Pattern disappears after closing the modal (courses)

The idea of this PR was to register the patterns only. This logic to apply was just a temporary function to simulate that it was working. It's being implemented in #5179

Wizard is not working when creating Lessons

For this, I saw that you are already working in a PR with the fix, right?

@renatho renatho requested a review from aaronfc May 26, 2022 12:17
@aaronfc
Copy link
Contributor

aaronfc commented May 26, 2022

Then... Looks good and works as described! 🚀

@renatho renatho merged commit 2e50a70 into feature/course-lesson-wizard May 26, 2022
@renatho renatho deleted the add/sensei-patterns branch May 26, 2022 14:23
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.

2 participants