Skip to content

Add template block to allow redefining the course details section in themes - #17543

Merged
tuchfarber merged 1 commit into
openedx:masterfrom
open-craft:clemente/upstream-add-block-to-course-about
Feb 26, 2018
Merged

Add template block to allow redefining the course details section in themes#17543
tuchfarber merged 1 commit into
openedx:masterfrom
open-craft:clemente/upstream-add-block-to-course-about

Conversation

@clemente

Copy link
Copy Markdown
Contributor

This PR is very similar to https://github.com/edx/edx-platform/pull/17091
It adds a <%block> that we needed to redefine in a theme.
The block name is prefixed with the template name to avoid clashes.

Pinging @tuchfarber (reviewer of that previous PR). Very simple PR.

JIRA tickets: N/A
Discussions: None
Dependencies: None
Screenshots:
Sandbox URL: None.
Partner information: N/A
Deployment targets: N/A
Merge deadline: None
Testing instructions: None required
Author notes and concerns: N/A
Reviewers

Settings
None

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @clemente! I've created OSPR-2266 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.

@jcdyer jcdyer 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.

Indentation on this file is misleading. Please fix the nesting of the <%block> marker. Otherwise, this is good to go.

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.

There's an indentation issue here. The </div> on line 221 closes the div.details on L211, not the div.container on L210. Move the <%block name="course_about_details"> inside the div.container

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing it. The original </div> already had the wrong indentation. I corrected it and also moved the %block inside the div.container as suggested.

@clemente
clemente force-pushed the clemente/upstream-add-block-to-course-about branch from b8e384e to 6288926 Compare February 26, 2018 09:20
@tuchfarber

Copy link
Copy Markdown
Contributor

Looks good to me. Will merge

@tuchfarber
tuchfarber merged commit e3b29af into openedx:master Feb 26, 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.

@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 Tuesday, February 27, 2018.

@clemente
clemente deleted the clemente/upstream-add-block-to-course-about branch February 26, 2018 18:18
@clemente

Copy link
Copy Markdown
Contributor Author

Thanks.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

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.

5 participants