-
Notifications
You must be signed in to change notification settings - Fork 324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: toggle button for switching between light and dark theme #97
Conversation
tutorindigo/templates/indigo/lms/static/sass/extra/_header.scss
Outdated
Show resolved
Hide resolved
bd971df
to
6e9cd62
Compare
@regisb Could you please go through it again? |
tutorindigo/templates/indigo/lms/templates/header/navbar-authenticated.html
Show resolved
Hide resolved
tutorindigo/templates/indigo/lms/templates/header/navbar-authenticated.html
Outdated
Show resolved
Hide resolved
The code implements the following conditions: LMS:
MFE (Micro-Frontend):
LMS and MFE Integration:We set |
|
||
tutor config save --set INDIGO_ENABLE_DARK_THEME=True | ||
tutor config save --set INDIGO_THEME_COOKIE_NAME=null | ||
tutor images build openedx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing left is to update the instructions :)
tutor config save --set INDIGO_ENABLE_DARK_TOGGLE=false
tutor local restart lms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the instructions:
tutor config save --set INDIGO_ENABLE_DARK_TOGGLE=false
tutor images build openedx # required after enable/disable toggle
tutor local start -d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh it's too bad that we have to rebuild the openedx image is there any way to avoid that, such as accessing the setting from the js code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are depending on rebuilding of image due to the following two reasons:
- JS file
- Scss file if condition to show/hide toggle. [This can be handled in js file too.]
I'm exploring it. So far, I'm unable to find a solution. Just thinking, if we can somehow pass variable from HTML/Mako to js . something like this: but this isn't working.
<%!
from django.conf import settings
%>
<%
indigoDark = settings.MFE_CONFIG.get('INDIGO_ENABLE_DARK_TOGGLE', true)
%>
<div class="theme-toggle-button mr-4">
.....HTML.....
</div>
<script type="text/javascript">
window.darkkTheme = '{{ indigoDark }}'
</script>
OR pass value to a tag and hide it like the below:
<%!
from django.conf import settings
%>
<%
indigo_dark = settings.MFE_CONFIG.get('INDIGO_ENABLE_DARK_TOGGLE', true)
%>
<div class="theme-toggle-button mr-4">
.....HTML.....
</div>
<span class='hide indigo-toggle-value'>{indigo_dark}</span>
then get value in js file using. $('indigo-toggle-value');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't spend too much time on this. If you can't find a quick fix, let's just ask people to rebuild the openedx image.
Pretty rad @hinakhadim! Let's fix the instructions and merge in master, such that Redwood can benefit from that feature. |
|
||
const AddDarkTheme = () => { | ||
const cookies = new Cookies(); | ||
const isThemeToggleEnabled = getConfig().INDIGO_ENABLE_DARK_TOGGLE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unable to see the toggle in the MFEs, so I'm guessing this is not working as expected. The LMS config API call is properly returning INDIGO_ENABLE_DARK_TOGGLE: true
. Can you please troubleshoot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, header package is not published yet in which we are using INDIGO_ENABLE_DARK_TOGGLE
. Here's the PR for header. I am currently testing the header package and then publish it.
Then, I have to update header package version here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Publishing the header is done and the version is updated too. I've tested on my system and openedx and mfe image building is successful and working. I will test on sandboxdev and inform the results here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested on sandboxdev and it's working fine. Steps I've taken:
tutor local stop
rm -rf tutor/env
pip install git+https://.......this branch
tutor config save
tutor images build openedx
tutor images build mfe
tutor local start -d
tutor config save --set INDIGO_ENABLE_DARK_TOGGLE=false
tutor images build openedx
tutor local start -d
tutor config save --set INDIGO_ENABLE_DARK_TOGGLE=true
tutor images build openedx
tutor local start -d
c0b688e
to
5eeda59
Compare
CHANGELOG.md
Outdated
- [Feature] Introduced theme toggle feature with enable/disable option and runtime switch between light and dark modes. (by @hinakhadim) --> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove --> at the end
{% if INDIGO_ENABLE_DARK_TOGGLE %} | ||
$('body').toggleClass("indigo-dark-theme", theme === 'dark'); // append or remove dark-class based on cookie-value | ||
// update expiry | ||
$.cookie(themeCookie, theme, { domain: window.location.hostname, expires: 90, path: '/' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is this different from operation in above file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The applyThemeOnPage()
function is the minimum criterion directly affecting whether the dark theme is applied. Instead of adding to the whole file, we focus specifically on this function.
But there are no significant performance gains in either way because LMS has to compile JS file whenever building an image.
9835384
to
5057307
Compare
|
||
function applyThemeOnPage(){ | ||
const theme = $.cookie(themeCookie); | ||
{% if INDIGO_ENABLE_DARK_TOGGLE %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use django template if the condition here? It is JS file and not being included in django template such that the context variables will work here.
tutorindigo/templates/indigo/lms/static/sass/partials/lms/theme/_variables.scss
Outdated
Show resolved
Hide resolved
tutorindigo/templates/indigo/lms/templates/header/theme-toggle-button.html
Outdated
Show resolved
Hide resolved
5057307
to
0b4aa10
Compare
This PR introduces toggle theme between light and dark on runtime in 5 MFEs (learner-dashboard, learning, account, profile, discussions):
Architecture:
MFEs:
@edly-io/indigo-brand-openedx@^2.1.0
includes SCSS files supporting light and dark themes.@edly-io/indigo-frontend-component-header@^3.1.3
provides an option to show or hide the theme toggle icon based on the presence of MFE_CONFIG['INDIGO_ENABLE_DARK_TOGGLE'].env.config.jsx
file that loads the theme in the MFE by reading the cookie value to determine which theme is to be applied.LMS:
dark-theme.js
loads the cookie value when the page is first loaded.