Add support for adding and changing Themes-- and dark mode - #566
Conversation
…ke dark mode/themes changes.
MikeNeilson
changed the title
|
@wjonassen FYI plan to merge this in sometime Monday morning (Pacific) baring any additional PRs with related code that was changed. |
krowvin
left a comment
krowvin
left a comment
There was a problem hiding this comment.
Few comments, but I did not pull this down and run it. Let me know if you'd like me to do an actual test.
As for the code itself, if it works I wouldn't call any of them blockers. You decide. But things like the global css on .active could bite us.
| <li> | ||
| <button type="button" class="dropdown-item d-flex align-items-center" data-bs-theme-link="${theme.link}" aria-pressed="false"> | ||
| ${theme.name} | ||
| <button type="button" class="dropdown-item d-flex align-items-center ${theme.name.contains("Default") ? 'true' : 'false' }" data-bs-theme-link="${theme.link}" aria-pressed="${theme.name.contains("Default") ? 'true' : 'false' }"> |
There was a problem hiding this comment.
I spent a minute staring at this...
Am I reading correctly that you are using a css class called true and false to set the unchecked state?
I'm sure this works but i've never really seen it done this way. Does Bootstrap use this?
I would have thought this would have been used:
https://getbootstrap.com/docs/5.0/forms/checks-radios/
Unless there's a styling thing i'm missing.
Looking at the logic you have a dynamic list of themes. You loop through them. if it is the default theme it is checked, otherwise it is not.
There was a problem hiding this comment.
... wait, no, I think that should be active.
| <button type="button" class="dropdown-item d-flex align-items-center" data-bs-theme-link="${theme.link}" aria-pressed="false"> | ||
| ${theme.name} | ||
| <button type="button" class="dropdown-item d-flex align-items-center ${theme.name.contains("Default") ? 'true' : 'false' }" data-bs-theme-link="${theme.link}" aria-pressed="${theme.name.contains("Default") ? 'true' : 'false' }"> | ||
| ${theme.name} <svg class="bi ms-auto d-none theme-icon" aria-hidden="true"><use href="#check2"></use></svg> |
There was a problem hiding this comment.
Initially I did not know what check2 was for, then I recalled it's how the icons are referenced from the rendered svg elsewhere in the page.
For future re-use perhaps checkmark instead of check2?
There was a problem hiding this comment.
I considered it, may do in a follow up. Wanted to keep the elements copied from bootstrap example as close to the example as possible so I didn't just confuse myself.
There was a problem hiding this comment.
Oh I see it's the second checkmark in their icon list
I didn't know what two was for us
| width: 24px; | ||
| height: 24px; | ||
| width: 1.5em; | ||
| height: 1.5em; |
There was a problem hiding this comment.
Might be worth going through the entire codebase at some point and looking for fixed vs relative sizes (for fonts too)
| } | ||
|
|
||
| .active .bi { | ||
| display:block!important |
There was a problem hiding this comment.
Important when you have to, but you can get clever with css nesting to avoid important
Many things use active and you'll end up in a place where it bites you if you make all things .active be !important
This ends up being global
Perhaps you can do
.dropdown-menu .active {
display:block!important
}To afford some wiggle room and prevent something else later on down the line getting forced to a block?
| element.classList.remove('active') | ||
| element.setAttribute('aria-pressed', 'false') | ||
| }) | ||
| console.log("hello") |
There was a problem hiding this comment.
Toss your logs out imo on PR so they don't get lost, harmless but not very professional in the final product
| // } | ||
| const btnToActive = document.querySelector(`[data-bs-theme-link="${theme}"]`) | ||
|
|
||
| document.querySelectorAll('[data-bs-theme-link]').forEach(element => { |
There was a problem hiding this comment.
I'm probably missing it in this commit
But i would add a comment somewhere that this
// Resets the theme state for all clicked themes and applies the current user's selected themes
There was a problem hiding this comment.
no, that's a good idea. I copied that from bootstrap but it took me a few minutes as well.
|
2 participants
Problem Description
Fixes #562
Fixes #432
Describe the problem you are trying to solve.
Solution
Created SPI mechanism and jspf page to handle pulling themes from embedded jars.
Dark Mode Toggle
Adjusted modal background color classes to use generic bootstrap5 background classes instead of specific color
Made decodescript modal modal-xl; even on a 4k monitor it felt unusable.
Made all tables have stripes
Remove "OpenDCS -" prefix from every modal; if you don't know you're in OpenDCS by that point no additional text in a sub section header is going to help you.
NOTE: sidebar no longer collapses, will correct in followup PR.
Additionally corrected some incorrectly scoped jar dependencies
how you tested the change
Describe what was done to test the change. This section can be left blank
if automated tests demonstrating usage are provided in the PR.
Where the following done:
(Formerly called regression tests.)
If you aren't sure leave unchecked and we will help guide you to want needs changing where.