LMS template theming for Stanford 6/11/13 launch #36
Conversation
ghost
assigned
|
Just to do some basic testing myself, here's what I did:
Granted, this is mostly just testing that this isn't doing any harm, not really testing the feature itself, but it's still valuable. |
There was a problem hiding this comment.
@sefk had a good question about why I prefixed the theme template file (and all theme templates, actually) with theme-. The reason is that in some cases, we want the ability for theme templates to inherit from the base (edX default) templates. If we just plopped the theme templates into the TEMPLATE_DIRS search path ahead of the edX templates, (and didn't use a theme- prefix) then we'd lose this ability as the template lookup would only find the theme templates.
What would be really nice would be a way to namespace the theme template files, just like you can do with static files. The way to do this right now would be to have a theme directory structure like themes/<theme-name>/templates/theme/<theme-name>/, which sucks. I guess you could probably also hack it with symlinks, but that's nasty too.
|
So I still need to read this over properly, but a quick question re: "the most obnoxious part" -- would it make it simpler to have the Python side create/update a .gitignore'd symlink to point to whatever theme is being used, and have the rakefile watch that? Honestly not sure what's ickier at this point. |
|
Hmm, clever idea. That could be really nice for local dev environments. Also, Rake is invoked before Django, so I'm not sure how Python would On Saturday, June 1, 2013, David Ormsbee wrote:
Nate |
|
I take that last part back. We could do this in the asset preprocessing On Saturday, June 1, 2013, Nate Hardison wrote:
Nate |
lms/envs/aws.py
Outdated
There was a problem hiding this comment.
Could we put a default value for this as well?
There was a problem hiding this comment.
There's a default in https://github.com/edx/edx-platform/blob/6f9a005957a4de660927cae29e4be3d62a016645/lms/envs/common.py#L35, if that's what you mean. I figured the PLATFORM_NAME was a must-have in the env.json file, much like SITE_NAME on https://github.com/edx/edx-platform/blob/6f9a005957a4de660927cae29e4be3d62a016645/lms/envs/aws.py#L104
There was a problem hiding this comment.
I was mostly concerned about breaking various folks who already have servers standing up internally on AWS (like MIT), since this is a new required entry. Given everything else defaults to edX at the moment anyway, could we just make this "edX" by default as well for now? @e0d, @jarv: Am I just being paranoid? I'm not sure who's running what outside of edX these days.
|
Is a full blown theming system one of the top priorities after 6/11? I know it's only there as a temporary expedient, but |
|
just to close the loop, I think most of this pull request is out of our area, aside from whatever fix needs to happen with the sass import file, so as far as i'm concerned it's a 👍 @ormsbee @cpennington - it's in your hands to approve. |
|
My immediate objections are just the use of |
|
Thanks @ormsbee @frrrances. I'll resolve the objections right after the meeting I'm sitting in now. |
Provide the appropriate switches to adjust based on whether or not a theme (in particular, the Stanford theme) is enabled in the settings. For now, these changes are very specific to Stanford. This is because the template architecture needs some reworking to generalize nicely.
The `FAVICON_PATH` setting determines the location of the favicon for the site. It's automatically adjusted when a theme is enabled, establishing the convention that themes will place their favicon in `static/images/favicon.ico`.
Allow themes to inherit from the default navigation bar and override pieces of it, including the main logo, the links that display to the right of the logo, and the links inside the dropdown menu (with the exception of the `Log Out` link. In addition, this adds an empty block at the very top so that themes can place a branding bar at the top of the page. (Stanford identity guidelines require this: see https://identity.stanford.edu.)
Adjust the now-defunct landing page so that it doesn't render much of the edX-specific marketing info (social links, press releases, university partners, etc.) if a theme is enabled. Additionally, if the Stanford theme is enabled, add in some school- specific language and adjust the video modal to play a Stanford one.
This setting is used to control the display name of the platform. The default is "edX", but themes may wish to override. For example, Stanford will use "Stanford Online" for the time being.
This mostly involves rewriting all mentions of "edX" to reference the `PLATFORM_NAME` setting instead. However, there are also some Stanford-specific rewrite hacks that need to be pulled out eventually. Additionally, don't display links to marketing pages (or the sections referencing those marketing pages) if the links are not defined by the theme.
As with the registration page, the bulk of the theming work here is replacing instances of "edX" with the `PLATFORM_NAME` setting. There is also a change to the "help" section, disabling it if the FAQ marketing link isn't set.
When a non-edX theme is enabled, then don't return anything for "top news," which is edX-specific.
Again, most of the work here is replacing "edX" with the `PLATFORM_NAME` setting. Need to ensure that the `news` boolean is indeed a falsy value as well, or just add a `theme_enabled()` test to disable the news block entirely (since news is an edX-specific feature).
Use the theme's own Google Analytics template (should probably update to just use parameters once the default GA template is kept up-to-date). Don't link to the university profile page when a theme is enabled, as that's an edX-specific feature. Adjust social links for Stanford, but leave them alone for everyone else (this is just a hack for the 6/11/13 launch).
Much like the work done on the default (unauthenticated) index view, adjust the background image (actually, let the CSS handle it instead of an embedded `style` attribute in the HTML). Other adjustments (language, logo) are made for Stanford specifically and need to be reworked for general theming.
Use the `PLATFORM_NAME` setting instead of "edX"
Configure the technical support email address in the settings so that themes can override with an email of their own in the appropriate env.json file in production.
Reference the `PLATFORM_NAME` and `TECH_SUPPORT_EMAIL` settings in the static error pages instead of hardcoded, edX-specific values.
Allow themes to override contact and bugs email addresses via the settings.
Remove the Stanford-specific if/else hack to set the appropriate contact email address and use the `CONTACT_EMAIL` setting instead.
Instead of hardcoding things like the platform name, use the corresponding overrideable settings instead. This allows themes to control emails as well.
Update the email change templates to fit with the rest of the main site and use the standard notification template. Now they're far prettier than before.
Adjust language of LMS emails to work specifically for Stanford. Here there be dragons: lots of ugly conditionals that will need to be changed once we develop a way to "theme" arbitrary strings throughout the site.
Since the theme Sass is just a simple variables overrides (for the moment), it must be imported right after the default variables Sass file (if at all). At the bottom of the file, it has no effect.
Just as is done with the main LMS application.scss file, rewrite the course.scss file with Mako to conditionally import a theme's variables overrides. Add the course.scss file to the list of ignored Git files so that it doesn't keep getting committed over and over again. This also requires us to add a hardcoded line in the assets Rakefile for the moment, so that the course.scss.mako file gets properly preprocessed. Once the preprocessing is done by a Django management command, we won't have to do this anymore.
Themes do not necessarily want all of the available LMS routes, such as `/jobs` and `/university_profiles`. This change splits up the `lms/urls.py` file and selectively enables/disables routes based on whether or not a theme is enabled. This is a naive solution for now; a better solution gives themes a way to selectively overrides such routes. Additionally, with the `MKTG_URL_LINK_MAP` setting that hits certain routes immediately on each page render (whenever the `marketing_link` helper function is called), themes may crash if they don't leave all marketing link routes present in `lms/urls.py`. This change also adds an `UNUSED_MKTG_URLS` setting that deletes unwanted URLs from the `MKTG_URL_LINK_MAP`.
Rather than require the `PLATFORM_NAME` to be specified in the `ENV_TOKENS`, allow the default setting in `common.py` to be used.
Instead of using the `UNUSED_MKTG_URLS` setting to selectively delete keys from the `MKTG_URL_LINK_MAP`, perform a full override of the map. Additionally, clean up how this map is parsed in `lms/urls.py`, making certain assumptions about how the map keys relate to the desired URLs and the corresponding template names. Finally, modify the mitxmako marketing URL middleware to not try to reverse disabled URLs, which are those keys in the map whose values are `None`.
Minor stylistic tweak.
|
All right: rebased onto latest master and addressed @ormsbee's objections. @cpennington @ormsbee, any parting thoughts? Also, rather than have this PR and #28 be separate merges (since #28 depends on this one), would you rather I rebase #28 on top of this one and update this PR (or open a new one)? If not, we should merge this one first. |
|
Works for me. I'd probably rebase #28, but as long as course.scss doesn't reappear, I'm happy. |
|
The way I read this @frrrances and @ormsbee have blessed this branch. Are we clear to click the "Merge" button? Any additional testing needed? @mikigoyal had asked for this branch to be put up on a sandbox machine for others to look at, did this happen? |
There was a problem hiding this comment.
Not blocking, but FWIW, this can just be be written as MKTG_URL_LINK_MAP.update(ENV_TOKENS.get('MKTG_URL_LINK_MAP', {}))
|
Closing in favor of pull request #57. |
…g_auth to amplify-master * commit '4954bde7c9578a1f6e97efb67acd671a941f709a': Additional changes to login flow to make it work correctly on POC and non local environments
* clear-cookies: remove unecessary white spaces [#118165513] clear session for users not yet logged in
* xblock initialization fixed and removed iframe * label and basic style fixes
Add support for multi-org sites
…pport feat!: dropped python 3.8 support
For @cpennington @ormsbee @frrrances @talbs @markchang @sefk @jinpa @caesar2164
Overview
This is based on last week's conversation, and it is the branch that I'd like to be pulled into
master. This is a companion to @caesar2164's changes in #28 in that both are needed for Stanford's 6/11/13 launch. (We're in the process of putting together one branch combining both PRs so that you can review/pull it all at once, if you so choose.)As we talked about in the RFC PR in the pre-clean repo, this PR is not a work of art. There are a number of ugly conditionals and hacks that are both Stanford- and edX-specific. However, the idea is that a compromise for now will get us living on
masterand better able to turn this into a full-blown theming layer in the near future. I have tried to pull things out into the settings where possible, but there are a few places (e.g., thelms/templates/emails/) where some Stanford-specific hacking was needed to get the desired language in place.Installation
First, you'll need to grab our theme files, located in a public repo at https://github.com/Stanford-Online/edx-theme. We've established a convention that theme files will live outside of the main edx-platform repository, in the
edx_base/themes/<theme-name>(ormitx_all/themes/<theme-name>) directory. To grab the Stanford theme, assuming you're inside your repository dir (edx-platform/), run:Probably the most obnoxious part of themes is that both Django and Rake need to know:
Django needs to know for obvious reasons (adjusting settings like
STATICFILES_DIRS,MAKO_TEMPLATES, etc. and for rendering templates appropriately), and Rake needs to know because it controls static asset compilation. If a theme is enabled, we need to conditionally import the theme's Sass overrides into the main LMS Sass files (lms/static/sass/{application,course}.scss), and we need to set the Sass compiler's load and watch paths appropriately so that the theme's Sass files are included in compilation. Unfortunately, Sass does not support conditional imports, so we instead turn the main LMS Sass files into Mako templates, and use Mako to control the conditional theme import. This Mako invocation is something that must also be done by the Rakefile, since it must be done before the Sass compiler runs.Right now, this is one nasty hack in(#5 is now merged intoassets.rake, but #5 does this in a much better fashion.master.)Anyway, the long and the short of all this is that we cannot just turn a theme on and off with Django settings, since Rake cannot parse a Django settings file. Therefore, as is done in production environments, we use a JSON file located in the
ENV_ROOT(the parent dir of your repo dir) to set the theme settings. The JSON file in dev environments is simply calledenv.json; in production, it's named slightly differently. If you want to turn a theme on, then you must have this file (or Rake won't invoke Mako and set up Sass properly), and if you want to disable the theme, then you either cannot have this file (or else Rake will invoke Mako and set up Sass to load the theme's Sass) or you have to edit out its theme setting. For simplicity, I usemv env.json{,.bak}andmv env.json{.bak,}to "disable" and "enable" this file (respectively) as needed.My
env.jsonlooks like this (edited on 6/3 to reflect updates to how we override marketing URLs):The
THEME_NAMEsetting is the important one, as it's thethemes/<theme-name>directory that Rake looks for when compiling assets (Django will also reference it too). The others are standard overrides that you could just as well specify in your customlms/envs/<settings>.pyfile. All of the other settings, however, with the exception ofSITE_NAME, are new in this PR.My Django settings file mimics
lms/envs/aws.pyclosely, since I wanted to test that my changes toaws.pyworked. Here's what it looks like, in case you want to copy it in for testing this PR (also edited on 6/3 to reflect marketing URL override updates):Therefore, I boot up my LMS with
rake lms[dev_stanford], and life is good.Settings changes
I added a few new settings in this PR that folks who deploy should know about, in case they want to override them in the
*_vars.ymlfiles (looking at you guys, @jbau, @e0d, and @jarv). Here they are:PLATFORM_NAMEis the display name of the platform (e.g., "edX" or "Stanford Online")UNUSED_MKTG_URLSallows environments to disable unwanted URLs in theMKTG_URL_LINK_MAP(e.g., themes don't want aJOBSpage)TECH_SUPPORT_EMAILis what it says (e.g., "technical@edx.org"). Likewise, we also have:BUGS_EMAIL(e.g., "bugs@edx.org")CONTACT_EMAIL(e.g., "info@edx.org")There's also a
FAVICON_PATHsetting, but that'll be automatically overridden when a theme is turned on.The appropriate defaults for all of these settings should be set in
lms/envs/common.py, and I added overriding code tolms/envs/aws.py. Please let me know if I missed anything.TODO
As mentioned above, this PR does leave something to be desired in terms of a better theming layer. I put comments in the code where I think the hacks are particularly heinous. Hopefully we'll have time to revisit this soon.
Additionally, there are still a couple of things I see that we have to do in order to get this ready for 6/11:
lms/templates/registration/password_reset_{confirm,email}.html,lms/templates/main_django.html). I would appreciate advice on how to do this, as from what I can tell, thesettingsare not made available to these templates' contexts. And since arbitrary Python in Django templates isn't possible by default (I think? Unfamiliar here...), I think we have to do something like this. Themain_django.htmltemplate is blocking, since that's the base template for the wiki.