Skip to content
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

Consume more tokens from arches theme, add dark mode toggle #91

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
4 changes: 2 additions & 2 deletions .github/actions/build-and-test-branch/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ runs:
- name: Ensure frontend configuration files exist
run: |
export ARCHES_DJANGO_DEBUG=True
python manage.py check
python manage.py check --tag=compatibility
shell: bash

- name: Install Arches applications
Expand Down Expand Up @@ -75,7 +75,7 @@ runs:

- name: Check for missing migrations
run: |
python manage.py makemigrations --check
python manage.py makemigrations --check --skip-checks
shell: bash

- name: Ensure previous Python coverage data is erased
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Added
- Add login interface [#13](https://github.com/archesproject/arches-lingo/issues/13)
- Add front-end router [#11](https://github.com/archesproject/arches-lingo/issues/11)
- Add dark mode toggle [#91](https://github.com/archesproject/arches-lingo/issues/91)
- Add backend for search [#67](https://github.com/archesproject/arches-lingo/issues/67)

### Fixed
Expand Down
10 changes: 9 additions & 1 deletion arches_lingo/src/arches_lingo/App.vue
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,15 @@ router.beforeEach(async (to, _from, next) => {
</div>
</div>
</main>
<Toast />
<Toast
:pt="{
summary: { fontSize: 'medium' },
detail: { fontSize: 'small' },
messageIcon: {
style: { marginTop: 'var(--p-toast-messageicon-margintop)' },
},
}"
/>
</template>

<style scoped>
Expand Down
45 changes: 45 additions & 0 deletions arches_lingo/src/arches_lingo/components/header/DarkModeToggle.vue
Copy link
Contributor

@chrabyrd chrabyrd Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<script setup lang="ts">
import { onMounted, ref } from "vue";
import { useGettext } from "vue3-gettext";

import ToggleButton from "primevue/togglebutton";

import { DEFAULT_THEME } from "@/arches/themes/default.ts";

const { $gettext } = useGettext();

const isDarkModeEnabled = ref(
window.matchMedia &&
window.matchMedia("(prefers-color-scheme: dark)").matches,
);

function toggleDarkMode() {
chrabyrd marked this conversation as resolved.
Show resolved Hide resolved
const element = document.querySelector("html");
// Remove the leading dot from the selector to get class name.
element!.classList.toggle(
DEFAULT_THEME.theme.options.darkModeSelector.substring(1),
);
isDarkModeEnabled.value = !isDarkModeEnabled.value;
}

onMounted(() => {
if (isDarkModeEnabled.value) {
toggleDarkMode();
}
});
</script>

<template>
<ToggleButton
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
:model-value="isDarkModeEnabled"
:on-label="$gettext('Dark')"
:off-label="$gettext('Light')"
off-icon="pi pi-sun"
on-icon="pi pi-moon"
:pt="{
root: { ariaLabel: $gettext('Toggle dark mode') },
label: { style: { display: 'none' } },
}"
@click="toggleDarkMode"
/>
</template>
18 changes: 12 additions & 6 deletions arches_lingo/src/arches_lingo/components/header/PageHeader.vue
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { useGettext } from "vue3-gettext";
import Menubar from "primevue/menubar";

import { routeNames } from "@/arches_lingo/routes.ts";
import DarkModeToggle from "@/arches_lingo/components/header/DarkModeToggle.vue";
import UserInteraction from "@/arches_lingo/components/user/UserInteraction.vue";
import SearchDialog from "@/arches_lingo/components/header/SearchDialog.vue";

Expand All @@ -21,15 +22,15 @@ const items = ref([

<template>
<Menubar
class="page-header"
:model="items"
style="border-radius: 0"
>
<template #start>
<RouterLink
:to="{ name: routeNames.root }"
style="text-decoration: none; color: inherit"
>
<h2>{{ $gettext("Arches Lingo") }}</h2>
<h1>{{ $gettext("Arches Lingo") }}</h1>
</RouterLink>
<SearchDialog />
</template>
Expand All @@ -43,18 +44,23 @@ const items = ref([
:class="item.icon"
aria-hidden="true"
></i>
<span>{{ item.label }}</span>
<span style="font-weight: var(--p-button-label-font-weight)">{{
item.label
}}</span>
</RouterLink>
</template>
<template #end>
<UserInteraction />
<div style="display: flex; align-items: center; gap: 1rem">
<DarkModeToggle />
<UserInteraction />
</div>
</template>
</Menubar>
</template>

<style scoped>
.page-header {
border-radius: 0;
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
:deep(.p-menubar-start) {
gap: var(--p-menubar-gap);
}

@media screen and (max-width: 960px) {
Expand Down
6 changes: 6 additions & 0 deletions arches_lingo/src/arches_lingo/components/login/LoginLinks.vue
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import { useGettext } from "vue3-gettext";

import Button from "primevue/button";

import { SECONDARY } from "@/arches_lingo/constants.ts";

const { $gettext } = useGettext();
</script>

Expand All @@ -13,11 +15,15 @@ const { $gettext } = useGettext();
as="a"
:label="$gettext('Register')"
:href="arches.urls.signup"
:severity="SECONDARY"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to deemphasize these buttons.

:outlined="true"
/>
<Button
chrabyrd marked this conversation as resolved.
Show resolved Hide resolved
as="a"
:label="$gettext('Multi-factor login')"
:href="arches.urls.auth + '?next=/'"
:severity="SECONDARY"
:outlined="true"
/>
</div>
</template>
6 changes: 3 additions & 3 deletions arches_lingo/src/arches_lingo/components/sidenav/SideNav.vue
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,13 @@ const items = ref([
display: flex;
flex-direction: column;
align-items: center;
min-width: 3rem;
min-width: var(--p-button-icon-only-width);
chrabyrd marked this conversation as resolved.
Show resolved Hide resolved
border-right: 1px solid var(--p-menubar-border-color);
}

.p-button {
min-height: 3rem;
min-width: 3rem;
min-height: var(--p-button-icon-only-width);
min-width: var(--p-button-icon-only-width);
border-radius: 0;
font-size: 1.5rem;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,11 @@ const greeting = computed(() => {
</script>

<template>
<div style="display: flex; align-items: center">
<div style="display: flex; align-items: center; gap: 1rem">
<span v-if="user">{{ greeting }}</span>
<Button
style="margin-left: 1rem"
:label="$gettext('Sign out')"
Copy link
Member Author

@jacobtylerwalls jacobtylerwalls Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without using the label prop, it wasn't using the tokens for button label font weight (not noticeable until I started overriding that weight via the theme)

@click="issueLogout"
>
{{ $gettext("Sign out") }}
</Button>
></Button>
</div>
</template>
1 change: 1 addition & 0 deletions arches_lingo/src/arches_lingo/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type { UserRefAndSetter } from "@/arches_lingo/types.ts";

export const ANONYMOUS = "anonymous";
export const ERROR = "error";
export const SECONDARY = "secondary";

export const DEFAULT_ERROR_TOAST_LIFE = 8000;
export const SEARCH_RESULTS_PER_PAGE = 25;
Expand Down
Loading