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

Create 3 default lessons on the Course Outline Block #5196

Merged

Conversation

fjorgemota
Copy link
Member

@fjorgemota fjorgemota commented May 26, 2022

Fixes #5142

Changes proposed in this Pull Request

  • Change Course Outline block behavior to add three lessons (Lesson 1, Lesson 2 and Lesson 3) to the course by default, but only when the post is new;

Testing instructions

  1. Switch to this branch and run npm start (or install the plugin build on this PR on some WordPress installation);
  2. Create a new course;
  3. Verify that 3 lessons (Lesson 1, Lesson 2 and Lesson 3) appear on the course outline by default;
  4. Verify that if you save the course, the lessons will be saved too;
  5. Verify that if you delete some the lessons (or add new ones), the lessons are persisted correctly; (e.g the system doesn't replace the modified lesson list for the default lesson list)
  6. Verify that if you delete all the lessons, the Course Outline Placeholder will appear;
  7. Verify that, in a scenario where you save the course after deleting all the lessons, if you refresh the page, the three default lessons ARE NOT restored;

Screenshots

Screenshot of the course outline block with titles in each lesson used as placeholder

Note

I'm not 100% sure if this PR covers all the behaviors that we are expecting from issue #5142, but I 100% feel like this is a good starting point to start experimenting and working on that issue.

@fjorgemota fjorgemota added this to the Course and Lesson Wizard milestone May 26, 2022
@fjorgemota fjorgemota self-assigned this May 26, 2022
@fjorgemota fjorgemota changed the base branch from trunk to feature/course-lesson-wizard May 26, 2022 22:54
@fjorgemota fjorgemota marked this pull request as ready for review May 26, 2022 23:00
@fjorgemota fjorgemota requested a review from a team May 26, 2022 23:00
Comment on lines 78 to 80
{ type: 'lesson', title: __( 'Lesson 1', 'sensei-lms' ) },
{ type: 'lesson', title: __( 'Lesson 2', 'sensei-lms' ) },
{ type: 'lesson', title: __( 'Lesson 3', 'sensei-lms' ) },
Copy link
Member Author

Choose a reason for hiding this comment

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

By the way, I added translated strings here but I'm not 100% sure if we really need to give these "lessons" a title.

With this code, when I create a new lesson I see this:

Screenshot of the course outline block with titles in each lesson used as placeholder

Meanwhile, without the title, I see this:

Screenshot of the course outline block but without the titles

Which IMHO doesn't look bad either.

WDYT?

Also, would you implement this translation in any different way?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the "Lesson 1", "Lesson 2", "Lesson 3" text, personally. I think it makes it a bit more clear.

Copy link
Contributor

@renatho renatho left a comment

Choose a reason for hiding this comment

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

Have you tried an approach to just update the base template to start with 3 lessons?
You can find it in Sensei_Course_Blocks::initialize_blocks.

@fjorgemota
Copy link
Member Author

Hey @renatho. Thanks for your review!

I just tested the approach that you suggested and, while it works, for now, I have a question: Do you think that this will work after the implementation of the patterns for the courses?

Like, based on my understanding, the patterns work by replacing the blocks used on the template (or, effectively, the template), so if the user chooses a pattern containing the course outline, the course outline block will appear as defined on the pattern.

In other words, by updating the $block_template in Sensei_Course_Blocks::initialize_blocks (thanks for these references, by the way, it's always helpful), I'm just changing the default "pattern", which may be replaced by a pattern chosen by the user using the editor wizard, that may also contain a course outline but without the "default" lessons (as defined on Sensei_Course_Blocks::initialize_blocks).

Meanwhile, with the current approach, even if the user chooses another pattern because this behavior is implemented inside the course outline block itself, the default lessons will always be there, without having to redefine the default lessons again and again...

WDYT?

@renatho
Copy link
Contributor

renatho commented May 27, 2022

@fjorgemota, good point! But I think we could also define the expected templates in the patterns. Maybe it also makes it more flexible in case we wanna give specific names to lessons in the patterns?

My main concern with the current approach is that we usually should try to avoid changes in the blocks while mounting them. It can cause issues with the changes history (undo/redo), which could be fixed with __unstableMarkNextChangeAsNotPersistent, but trying to avoid it is probably better. 😀 Does it make sense?

…g in the block

As suggested by Renatho during review.
@fjorgemota
Copy link
Member Author

Hey @renatho. Fair enough. I changed in 442941c the logic so the lessons are defined in the post type template, then, as you suggested.

Thanks! :)

@fjorgemota fjorgemota requested a review from renatho May 27, 2022 18:06
Copy link
Contributor

@renatho renatho left a comment

Choose a reason for hiding this comment

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

Looks good and works well! 🎉

@@ -1,21 +1,24 @@
/**
* WordPress dependencies
*/
import { InnerBlocks } from '@wordpress/block-editor';
import {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the improvements in this file! =)

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