WL-416: Multi-site comprehensive theming - #12620
Conversation
| 'codejail.django_integration.ConfigureCodeJailMiddleware', | ||
|
|
||
| # django current site middleware with default site | ||
| 'django_sites_extensions.middleware.CurrentSiteWithDefaultMiddleware', |
There was a problem hiding this comment.
See openedx/edx-django-sites-extensions#6 and edx/edx-platform#12555
|
@saleem-latif I took an initial look at this PR and I think it is in good shape -- happy to see some of the things we've learned through the Otto theming effort being used here as well. @douglashall if there is someone from your squad who would be a good first-round reviewer of this PR, it would be great to have another set of eyes to jump in since mine are pretty blurry at this point 😄 |
cb68d2b to
8c38784
Compare
|
@saleem-latif Are the test failures here do to a configuration repo change that is in progress? |
There was a problem hiding this comment.
This middleware does not need to be configured for Studio.
7371dfc to
e3dc5cb
Compare
saleem-latif
changed the title
|
@douglashall I have updated the code according to your feedback and tests are fixed now. Kindly take a look |
| `site` field is foreignkey to django Site model | ||
| `theme_dir_name` contains directory name having Site's theme | ||
| """ | ||
| site = models.ForeignKey(Site, related_name='themes') |
There was a problem hiding this comment.
Is there a real use case for a site having more than one theme? If not, this should be a OneToOneField.
There was a problem hiding this comment.
So this has come up a couple times and a OneToOneField does make sense except there was a great idea from @mtyaka during our initial effort of introducing the ability to 'stack' themes in a sort of cascading approach. We really like this idea and in order to build it we'd want the relationship to be a ForeignKey and not a OneToOneField.
There was a problem hiding this comment.
Tell me more about this stacking feature. Does that mean this model should have an order or depth field?
There was a problem hiding this comment.
The idea is that you might have a 'stack' of themes you want to apply, which could be layered on top of each other to fill in certain overrides without having to duplicate across single themes. And yes there would have to be some sort of z-index value (order, depth, rank, etc.) to describe the hierarchy. However we don't want to jump into that without taking some time to think about the data model further, so we're only including the ForeignKey field spec for now.
|
@saleem-latif @mattdrayer Could we get step-by-step instructions for setting up theming in LMS? |
|
I agree, but a quick look leads me to believe there isn't really a good place at the moment for including setup info for theming within the LMS project, unless maybe we add a README.md to the theming app itself? We have a larger task to update the information @ RTD with the new theming instructions. Currently there is not much available there and what does exist applies to the first version of the feature. We'll need to catch up with @edx/doc to determine when the updated instructions need to be posted @ RTD (I am assuming with the Eucalyptus named release). In the meantime I have logged WL-504 to publish a wiki page. |
|
@mattdrayer Re: setup docs, cool, even inline here in a comment would be great. Thanks! |
|
Okay, bringing in Sir @andy-armstrong on this PR -- third time's a charm, we promise! 😄 This branch should look fairly familiar, with some additional enhancements based on what we learned during our Otto theming efforts. Otto theming works great (we are using it in production now for the MITPE site) and we are looking forward to having this functionality in LMS as well. Fingers crossed!!! |
| @@ -0,0 +1,255 @@ | |||
| // studio - utilities - variables | |||
There was a problem hiding this comment.
We should consider adding theming tests for pattern library pages too, since we are starting to land a number of these. I'm okay with doing this in a follow-up PR.
|
@mattdrayer Is there a sandbox? I'd like to see what happens with Pattern Library pages, as well as the wiki (since it uses Django Templates). |
|
@saleem-latif @mattdrayer This looks great. Fingers crossed that it can stick its landing this time! 👍 I agree about the need for wiki documentation, as the community has been struggling with all the changes. One final comment: any ETA for previewing a site/theme through URL parameter (https://openedx.atlassian.net/browse/WL-434). I find myself needing this all the time when changing Pattern Library pages as I want to make sure that I haven't broken any of the existing themes (both on devstack and on sandboxes). |
|
jenkins run python |
2205520 to
ae4b1c3
Compare
048fb3f to
34a2f28
Compare
|
jenkins run python |
55e0704 to
d46a2dd
Compare
|
jenkins run a11y |
07630ed to
ed42bbf
Compare
2. Update COMPREHNSIVE_THEME_DIR to COMPREHENSIVE_THEME_DIRS 3. Update paver commands to support multi theme dirs 4. Updating template loaders 5. Add ENABLE_COMPREHENSIVE_THEMING flag to enable or disable theming via settings 6. Update tests 7. Add backward compatibility for COMPREHEHNSIVE_THEME_DIR
ed42bbf to
68312bd
Compare
|
jenkins run python |
1 similar comment
|
jenkins run python |
5 participants
Hi @mattdrayer, @douglashall , @asadiqbal08
Kindly review this PR, it contains add Comprehensive theming changes back to edx-platform after being removed in Revert PR#12116. It also adds the ability to specify multiple theme directories and to enable or disable theming via
ENABLE_COMPREHENSIVE_THEMINGflag.This PR requires corresponding changes in configuration (PR#3099), and will be merged after it.