Consume more tokens from arches theme, add dark mode toggle #91
Conversation
jacobtylerwalls
changed the title
|
looks like some webpack failures in the CI? |
|
That's expected, since this depends on themes/default.ts in archesproject/arches#11319. I could update this branch to depend on it, but that seems like avoidable churn? |
jacobtylerwalls
force-pushed
the
jtw/consume-tokens
branch
from
4afd684 to
7d99575
Compare
jacobtylerwalls
force-pushed
the
jtw/consume-tokens
branch
from
7d99575 to
81daf62
Compare
Let's do a follow up, could be good to onboard another team member onto theming stuff? |
Yowza! I guess... do you have any manual overrides of the dev/8.0.x arches theme? I know on your system you did some deep dives into css reset for vue, so I'm wondering if you had any artifacts from that lying around, like css layers or something. |
|
boo failing builds |
oh huh. after you pulled main into the branch things are working now 🎉 |
There was a problem hiding this comment.
isDarkModeEnabled works! (🎉 ). However, regardless of if its value, the theme when the page is loaded is always light. I was able to solve this locally via
onMounted(() => {
if (isDarkModeEnabled.value) {
toggleDarkMode();
}
});
However, this may not be the right solution. There's further complexity to add that may nullify that pattern. Namely, I can set the toggle to dark, see the change, refresh the page, and it's automatically set back to light. My guess is that either cookies or localstorage need to be leveraged to persist this through page refreshes.
If we wanted to get really fancy we could add a column to the User model to support preferred theme, but that's probably overkill at this point. And we'd still need something on the browser to persist for anonymous user so /shrug that's not an entirely baked idea.
If this component is changed to read from cookies/localstorage on mount, it'd be more readable to change the toggle method to accept an arg so it can be explicitly set the mode. This will also support adding other themes later. However I'm not married to this pattern, YAGNI and whatnot /shrug. Regardless, reading from cookies/localstorage nullifies the simple code laid out above.
But this reveals another titchy thing: we're handling all of theming in this component. If I have a cookie ( or bit of localstorage ) saved that says I prefer dark mode, I'd expect the application to handle this regardless of the presence or absence of the header / DarkModeToggle. Like in reports and whatnot. It could be argued that the ability to switch themes should be handled in this component of course, but the ability to query a preferred theme and apply it should be handled at a deeper level.
Perhaps even in core since it's touching the <html> element, but I don't feel strongly about that yet.
There was a problem hiding this comment.
Good ideas all around. Updated, should retest with archesproject/arches#11667.
I'll update the github action. Maybe we need the updateproject command to yell about this again when the github templates change? |
|
The actions are dense; better to just call out the tiny change in the upgrade instructions. |
IMO it should be automated, part of the |
|
👋 ! Just circling back on this re: where the conversation landed earlier. I believe it ended up being that some of this logic gets moved to core. Essentially the logic to derive the user scheme preference and set it on the The rest -- supporting multiple scheme preferences and adding a column to the |
jacobtylerwalls
force-pushed
the
jtw/consume-tokens
branch
from
822246c to
ad5a688
Compare
arches_lingo/src/arches_lingo/components/header/DarkModeToggle.vue
Outdated
Show resolved
Hide resolved
Fix header skip #87
jacobtylerwalls
force-pushed
the
jtw/consume-tokens
branch
from
7a0fe50 to
cea781b
Compare
There was a problem hiding this comment.
Looks good! And handles persistence through refreshes.
I don't have a great answer for hard-coding on the arches core side. Perhaps there isn't one, users will just need to deal with the fact that there's a single name for dark mode toggling.. Actually thought of something below
That said, I'd like to minimize hardcoding if we can get away from it. And reading DEFAULT_THEME from within the application doesn't cover all bases. I mucked around a bit and found something ( relatively ) graceful:
import { getCurrentInstance } from 'vue';
const { appContext } = getCurrentInstance();
const darkModeClass = appContext.config.globalProperties.$primevue.config.theme.options.darkModeSelector.substring(1);
This will at least handle for any selector for any theme fed into the application. And maybe we re-think where the dark mode class interaction is core-side once this gets moved out of core.
I could argue that instead of living in a script tag in base-root, it could live inside createVueApplication so it has access to classname the theme is using and can behave appropriately. And the class still would get added before application mount.
but yeah just navel gazing I guess -- happy to punt that until later. For the time being, if you're amenable to get this change into DarkModeToggle, then we can merge 😄
Fix header skip closes #87
Add dark mode toggle closes #91