Skip to content

Add some blocks to course_about.html - #96

Merged
clemente merged 1 commit into
opencraft-release/ginkgo.1-pearsonfrom
clemente/pearsonx-add-blocks-to-course-about
Jan 4, 2018
Merged

Add some blocks to course_about.html#96
clemente merged 1 commit into
opencraft-release/ginkgo.1-pearsonfrom
clemente/pearsonx-add-blocks-to-course-about

Conversation

@clemente

@clemente clemente commented Dec 22, 2017

Copy link
Copy Markdown

Now that the new template inheritance was merged (https://github.com/edx/edx-platform/pull/16856) we can redefine specific blocks.
This adds some block names to existing code so that themes can redefine this blocks easily (without having to redefine the rest of the page).
This handles just course_about.html

JIRA tickets: None
Discussions: None
Dependencies: None
Screenshots: None
Sandbox URL: None
Merge deadline: None

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. Start a server which includes this change: https://github.com/edx/edx-platform/pull/16856, cherry-pick the commit as needed.
  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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need an empty block right before the next block? Can we not include the content we want to add to course_info_before_header to the top of the header block?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok, I changed it as you said.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We will have to be careful with the naming. The main.html template (which this template inherits from)also (indirectly) contains a

element. It is currently not wrapped in a named block, but it could be in the future and would clash with theheaderblock from this template. We should name this one something likecourse_about_header`.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe it would make sense to prefix all blocks introduced by a template by the template name.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I renamed blocks to use course_about prefix.

@clemente clemente changed the title WIP: add blocks to course_about, and new template inheritance system Add some blocks to course_about.html Dec 27, 2017
@mtyaka

mtyaka commented Jan 4, 2018

Copy link
Copy Markdown
Member

This looks good @clemente, but we still need the template block overrides commit. The commit has been merged to edx/edx-platform/master, but not into opencraft-release/ginkgo.1-pearson. It might make sense to do it in a separate PR though.

This is good to go after you squash the commits. 👍

  • I tested this: I verified that the refactored page using named blocks locks and behaves the same as the original page.
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation N/A

@mtyaka

mtyaka commented Jan 4, 2018

Copy link
Copy Markdown
Member

We will also need to open an upstream PR with the same changes (unless the structure of the course_about.html page has changed significantly on master as compared to ginkgo).

@clemente

clemente commented Jan 4, 2018

Copy link
Copy Markdown
Author

but we still need the template block overrides commit.
@mtyaka #101

@clemente
clemente force-pushed the clemente/pearsonx-add-blocks-to-course-about branch from 5798bbf to 5913516 Compare January 4, 2018 13:12
@clemente

clemente commented Jan 4, 2018

Copy link
Copy Markdown
Author

We will also need to open an upstream PR with the same changes

@mtyaka https://github.com/edx/edx-platform/pull/17091

@clemente
clemente merged commit aea13a6 into opencraft-release/ginkgo.1-pearson Jan 4, 2018
@clemente
clemente deleted the clemente/pearsonx-add-blocks-to-course-about branch January 4, 2018 14:30
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.

2 participants