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

Handle lesson hooks for lessons which contain a block #3935

Merged
merged 6 commits into from Feb 1, 2021

Conversation

gikaragia
Copy link
Contributor

@gikaragia gikaragia commented Jan 27, 2021

Changes proposed in this Pull Request

  • Removes content that is provided by 'Contact Teacher' block and 'Lesson Actions' block.
  • Add-ons will be handled as part of another PR.
  • I also remove the contact teacher button as we have plans to make it available in the coming PRs.
  • As we are not dropping the lesson template, there are two hook calls that are removed, footer_quiz_call_to_action and send_message_link. Method send_message_link adds a notice which is going to be handled by the Contact Teacher block. Method footer_quiz_call_to_action does not add any notices which means that no other changes for notices are required.

Testing instructions

  • Add the lesson actions block to a lesson and observe that the old 'Contant Teacher' button and the lesson actions are not displayed anymore.
  • Remove the block and observe that old content appears in the frontend.
  • Add a 'Contact Teacher' block in a course, send a message and observe that the notice is displayed correctly.

@gikaragia gikaragia requested a review from a team January 27, 2021 12:05
@gikaragia gikaragia changed the title Add/handle lesson hooks Handle lesson hooks for lessons which contain a block Jan 27, 2021
@gikaragia
Copy link
Contributor Author

yscik
yscik previously approved these changes Jan 27, 2021
Copy link
Contributor

@yscik yscik 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!

Base automatically changed from add/lesson-action-blocks to master January 29, 2021 12:07
@gikaragia gikaragia dismissed yscik’s stale review January 29, 2021 12:07

The base branch was changed.

@gikaragia gikaragia force-pushed the add/handle-lesson-hooks branch 3 times, most recently from 6df8b76 to ab04e5c Compare January 29, 2021 12:35
@gikaragia gikaragia requested review from yscik and a team January 29, 2021 13:24
@gikaragia
Copy link
Contributor Author

It seems that when I moved the 'Contact Teacher' notice to the constructor I forgot to remove the previous one. This is now fixed.

renatho
renatho previously approved these changes Jan 29, 2021
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!
Just a small suggestion, not critical.

@@ -33,6 +33,11 @@ public function register_block() {
'render_callback' => [ $this, 'render_contact_teacher_block' ],
]
);

// phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Arguments used for comparison.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not critical, but WDYT to handle it separately since it's not related to the block register?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 606e985

@gikaragia gikaragia requested review from a team and renatho February 1, 2021 11:56
@donnapep donnapep added this to the 3.8.0 milestone Feb 1, 2021
Copy link
Contributor

@yscik yscik left a comment

Choose a reason for hiding this comment

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

👌

@gikaragia gikaragia merged commit 628fc5f into master Feb 1, 2021
@donnapep donnapep deleted the add/handle-lesson-hooks branch February 1, 2021 19:31
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.

None yet

4 participants