-
Notifications
You must be signed in to change notification settings - Fork 259
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
Use current dark/light mode for btns in labelauty #718
Conversation
Otherwise, the buttons stand out and don't match the rest of the requested theme. If for some reason there isn't a `data-bs-theme` on the document, default to light mode.
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.
CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
There were a few issues in the CodeQL check, but this is the only one introduced in this PR. Instead of directly taking the `data-bs-theme` from the DOM, we can simply check if the theme is dark, and use the string "dark". Otherwise, we use "light", which prevents any direct DOM XSS issues. https://cheatsheetseries.owasp.org/cheatsheets/DOM_based_XSS_Prevention_Cheat_Sheet.html The full CodeQL run: https://github.com/jhpyle/docassemble/pull/718/checks?check_run_id=19234176736
I am not sure that JavaScript should be used to change CSS classes based on light/dark mode. The user needs to be able to change the light/dark mode and see the changes immediately without having to reload the screen or retrigger JavaScript. The JavaScript solution might not work for labelauty lists that appear on the initial page the user sees, because of the way the mode gets activated by JavaScript code. There could be other issues as well that arise from using JavaScript to do CSS's job. The underlying problem is that Bootstrap is missing a feature, but the feature is apparently on its way: In the meantime there is a CSS workaround that users can implement easily themselves (copying and pasting the
|
I agree that it should be a CSS thing, but that's not how changing the theme works at the moment. If you change the browser theme / appearance, you have to refresh the DA page for it to take affect. I see what you mean though, changing the theme in multiple places vs having everything be dependent on |
In dark mode, the display templates still have a light background, and stand out far too much. This adds CSS to change the `bg-light` class for the collapse and display templates to the dark-mode colors when `data-bs-theme=dark`. See jhpyle/docassemble#718 for a discussion on a suggested fix for a similar issue, which resulted in the suggested workaround here.
In dark mode, the display templates still have a light background, and stand out far too much. This adds CSS to change the `bg-light` class for the collapse and display templates to the dark-mode colors when `data-bs-theme=dark`. See jhpyle/docassemble#718 for a discussion on a suggested fix for a similar issue, which resulted in the suggested workaround here.
Otherwise, the buttons stand out and don't match the rest of the requested theme. To avoid DOM XSS issues, we check if
data-bs-theme
is dark, and use the string "dark" if it is. Otherwise (if for some reason there isn't adata-bs-theme
on the document, or it's some string we don't recognize), we default to light mode.This, along with #717 fixes some issues with trying to make a dark mode theme.
Some images of the differences here:
Pre-PR radio buttons and checkboxes in dark mode:
Post-PR radio buttons and checkboxes in dark mode: