Skip to content

Add some blocks to course_about.html - #17091

Merged
tuchfarber merged 1 commit into
openedx:masterfrom
open-craft:clemente/upstream-add-blocks-to-course-about
Jan 11, 2018
Merged

Add some blocks to course_about.html#17091
tuchfarber merged 1 commit into
openedx:masterfrom
open-craft:clemente/upstream-add-blocks-to-course-about

Conversation

@clemente

@clemente clemente commented Jan 4, 2018

Copy link
Copy Markdown
Contributor

Now that https://github.com/edx/edx-platform/pull/16856/ was merged, themes can implement particular blocks in templates, without having to redefine the whole .html
For this we need the definition of the blocks to be redefined. This PR adds block names to some sections of course_about.html that we needed to rewrite.

JIRA tickets: None
Discussions: None
Dependencies: None
Screenshots: None
Sandbox URL: https://pr17091.sandbox.opencraft.hosting/ , https://studio-pr17091.sandbox.opencraft.hosting/
Merge deadline: None

Testing instructions:

Adding blocks should be harmless. But if you want to test it in practice and check the new block override system, these are the full testing instructions:

  1. Without this change (i.e. with your normal devstack server), go to http://localhost:18000/courses/course-v1:edX+DemoX+Demo_Course/about and save the HTML in a file
  2. If not using master, cherry-pick this: https://github.com/edx/edx-platform/pull/16856
  3. Apply the patch in this PR, go to the same URL and save to another file
  4. Compare files, the HTML should be the same (ignoring whitespace and django-debug-toolbar)
  5. Check that no blocks are defined inside other blocks
  6. Check that these blocks are like the rest of the other blocks. Or if you're not sure and want to test it, create a theme with a file lms/templates/course_about.html which redefines one of the blocks, e.g. <%block name="course_about_header">
  7. Check that block names don't conflict with other block names used in templates
  8. Check that names make sense

Reviewers

Author concerns: None
Settings: None

@mtyaka

mtyaka commented Jan 5, 2018

Copy link
Copy Markdown
Contributor

👍

  • I tested this: I verified that adding the blocks doesn't change the default design and that redefining the blocks from a custom comprehensive theme works correctly.
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @clemente! I've created OSPR-2056 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Jan 10, 2018
@nedbat

nedbat commented Jan 10, 2018

Copy link
Copy Markdown
Contributor

@tuchfarber Can you take this one also?

@tuchfarber tuchfarber left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me.

@tuchfarber
tuchfarber merged commit 78bc871 into openedx:master Jan 11, 2018
@openedx-webhooks

Copy link
Copy Markdown

@clemente 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

@clemente
clemente deleted the clemente/upstream-add-blocks-to-course-about branch January 11, 2018 19:01
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Monday, January 15, 2018.

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

Labels

merged open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants