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

Apply pattern to the post when selected #5179

Merged
merged 21 commits into from
May 30, 2022

Conversation

renatho
Copy link
Contributor

@renatho renatho commented May 24, 2022

Fixes #5140
Fixes #5130
Fixes #5133

Changes proposed in this Pull Request

  • This PR has some general fixes (sorry for that in this PR), but the main goal is to apply the pattern correctly when it's chosen.
  • It also finalizes the pattern steps for course and lesson with the proper styles and content.

Testing instructions

  • Create a new course.
  • In the wizard form, add a title and a description.
  • Navigate through the steps until the "Course Layout".
  • Click in a pattern.
  • Make sure it's applied to the course with the title and description replaced, and the course is saved.
  • Repeat the test, but close the modal instead of choosing a pattern.
  • Make sure the modal is closed, the default template is used in the course, and the course is saved.
  • Also make sure that the pattern selection work on a new lesson.

Screenshot / Video

Screen.Recording.2022-05-26.at.16.21.09.mov

@renatho renatho added this to the Course and Lesson Wizard milestone May 24, 2022
@renatho renatho self-assigned this May 24, 2022
Base automatically changed from add/sensei-patterns to feature/course-lesson-wizard May 26, 2022 14:23
@renatho renatho force-pushed the add/pattern-selection branch 2 times, most recently from 98af7e0 to 828a1ae Compare May 26, 2022 17:35

// We can call `onCompletion` to complete the wizard after setting the correct pattern with `setData`.
// We could replace `onCompletion` with the `goToNextStep` callback with a similar effect.
if ( data.courseTitle ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we implement the final patterns, if we don't have a course title to replace, we can remove this condition.

@renatho renatho requested a review from a team May 26, 2022 19:41
@renatho renatho marked this pull request as ready for review May 26, 2022 19:41
aaronfc
aaronfc previously approved these changes May 27, 2022
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 good!
Just some clarifications:

  • Lesson step will need to be implemented after this is integrated, right?
  • I saw that Course Outline is added with some default lessons, which is nice, but it is not when using the default template (course outline appears without lessons) – this will be fixed here, right?

assets/admin/editor-wizard/steps/lesson-patterns-step.js Outdated Show resolved Hide resolved
assets/admin/editor-wizard/wizard.js Outdated Show resolved Hide resolved
/>
</div>
) }
</div>
</div>
) ) ||
null
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember needing to add this || null because I was getting an error that a render should return something always. Why it is not needed anymore?

Copy link
Contributor Author

@renatho renatho May 27, 2022

Choose a reason for hiding this comment

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

Ah, I got it! If steps[ currentStepNumber ] is undefined it would fire an error. But if we return null we also have a weird behavior with the modal open, but only with the overlay.

Thinking a little more, I don't think we need this check since goToNextStep will only increment if we have steps. I removed it for now in this commit: d64d74e, but let me know if I'm missing anything. 😉

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 also removed the conditional in the other case where steps should always be an array.

@renatho
Copy link
Contributor Author

renatho commented May 27, 2022

Thank you for your review, @aaronfc !

Lesson step will need to be implemented after #5194 is integrated, right?

Definitely! Good catch! I already updated the branch, since that was merged, and added the implementation here: 24207ca

I saw that Course Outline is added with some default lessons, which is nice, but it is not when using the default template (course outline appears without lessons) – this will be fixed #5196, right?

That's correct! Also about the format of the Outline in the patterns, it's something temporary, I think it will depend on how we define them.

@renatho renatho requested a review from aaronfc May 27, 2022 14:10
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 good works well :D

@renatho renatho merged commit 8f543a0 into feature/course-lesson-wizard May 30, 2022
@renatho renatho deleted the add/pattern-selection branch May 30, 2022 13:55
@alexsanford
Copy link
Contributor

When I test this (on the feature branch), and either close the modal or click "Start with default layout" (rather than selecting a pattern), it seems like the description that I've added to the modal is lost (it's not added to the post at all). Is this intentional? Should we have some way of adding the description to the default template? Or are we ok with just losing the description if the modal is closed?

My thought is we should have the description in the default template somewhere.

cc @renatho @burtrw

@renatho
Copy link
Contributor Author

renatho commented May 30, 2022

it seems like the description that I've added to the modal is lost (it's not added to the post at all). Is this intentional?

Good point! For now, it was intentional, just applying the description to the patterns. And when skipping, it just loads the default template. But I think it makes sense. Maybe we could add a paragraph with the filled description.

If we go for that, I have a question if the user doesn't add any information to the description input. In the case of the patterns, we're keeping the original content of the pattern. Maybe should we also have a paragraph with an example text in the default template?

@burtrw
Copy link
Member

burtrw commented May 30, 2022

I was thinking the description could/should also get applied as the Course 'excerpt' if that is possible?

But adding a simple text block at the top of the default that includes the description would also be great.

We could either just put a blank paragraph block where the description would go or have default text. My concern with default text is translations (though we are adding default text to other patterns...).

What do y'all think would be best?

@renatho
Copy link
Contributor Author

renatho commented May 31, 2022

I was thinking the description could/should also get applied as the Course 'excerpt' if that is possible?

Yep. It's possible to do it in a similar way to how we update the title currently.

But adding a simple text block at the top of the default that includes the description would also be great.

We could either just put a blank paragraph block where the description would go or have default text. My concern with default text is translations (though we are adding default text to other patterns...).

What do y'all think would be best?

IMO, I think since it's the default template and not a styled pattern, we could just leave an empty paragraph if the user doesn't inform a description. We could have a placeholder there how we have in the current lesson editor "Write lesson content..."

Screen Shot 2022-05-31 at 11 23 31

I wait for confirmation or a new suggestion to open an issue for that. 😉

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