Bug: Theme switcher element and labeling is incorrect

[thatblindgeye]

Complete the following REQUIRED checkboxes:

• [x] I have thoroughly read and understand The Odin Project Contributing Guide
• [x] The title of this issue follows the Bug: brief description of bug format, e.g. Bug: Lesson complete button does not update on click

The following checkbox is OPTIONAL:

• [ ] I would like to be assigned this issue to work on it

---

1. Description of the Bug:

The theme switcher has a few issues:

• The main element is currently an anchor element, when it should be either a button or checkbox (checkbox might be easiest in terms of output). Using NVDA it announces the switcher as "clickable link graphic theme icon". The switcher is not a link so should not be using an anchor element.
• The label of the switcher should not be "theme icon", as it doesn't convey any useful information. How this is updated depends on how the switcher element is updated. If we were to style a checkbox, we could just have the checkbox label be a static "Light theme" or "Dark theme" (whichever the default is), that way it would be announced as 'X theme checked/unchecked".
• The svg icon should have the title element removed, and the switcher should no longer be labelled based on it (remove the aria-labelledby
• Not really related to the main issue, but there is a significant delay between clicking the switcher and the icon updating. We should ideally try to make it more instantaneous.

2. How To Reproduce:

3. Expected Behavior:

4. Desktop/Device:

• Device:
• OS:
• Browser:
• Version:

5. Additional Information:

[KevinMulhern]

Thanks @thatblindgeye, the previous issue we had about this: https://github.com/TheOdinProject/theodinproject/issues/3497

[luuu-xu]

@thatblindgeye @KevinMulhern I would love to work on this. While I was working on other a11y issues before I have noticed the theme switcher's weird a11y issue but did not immediately come up with an idea to fix it. But it seems a bit complicated since it is a link to turbo method, what should we do?

[thatblindgeye]

@KevinMulhern any input on the above? Is there a reason we're using a link with a turbo method in the first place?

[KevinMulhern]

@thatblindgeye yep Turbo is what allows us to switch themes without a page refresh - but changing the element to a button shouldn't be an issue - it'll still work.

The label of the switcher should not be "theme icon", as it doesn't convey any useful information. How this is updated depends on how the switcher element is updated. If we were to style a checkbox, we could just have the checkbox label be a static "Light theme" or "Dark theme" (whichever the default is), that way it would be announced as 'X theme checked/unchecked".

This is likely the hardest problem to solve, we're limited with how much space we have in the navbar. More-so now we have the support link. But we might be able to do something like this:

Not really related to the main issue, but there is a significant delay between clicking the switcher and the icon updating. We should ideally try to make it more instantaneous.

Leave this one with me, there are a few optimisations we can make.

[thatblindgeye]

yep Turbo is what allows us to switch themes without a page refresh - but changing the element to a button shouldn't be an issue - it'll still work.

Gotcha, wasn't sure if there was any other reason. We should be able to just update the class of our html element onClick of a button then.

This is likely the hardest problem to solve, we're limited with how much space we have in the navbar. More-so now we have the support link. But we might be able to do something like this:

Whether we'd want to adjust that top navbar could be another discussion, but we could definitely keep the theme switcher as just an icon for now. Implementing a dropdown to toggle open (rather than simply switching between light and dark) would be a nice to have as well though maybe that could be a followup to this issue (unless @luuu-xu really wants to dig into that)

Leave this one with me, there are a few optimisations we can make

👍🏼 if it's any consolation I think the issue wasn't as bad on Chrome (I had noticed it more on Firefox).

[KevinMulhern]

We should be able to just update the class of our html element onClick of a button then.

We'll still need to hit that endpoint when clicking the button unfortunately, it persists the users theme choice so it won't change back to the default when navigating to other pages.

[KevinMulhern]

@thatblindgeye I've made a few optimisations in this PR. Theres still a little bit of delay because it needs to make a server round trip, but the icon and theme will change at the same time now.

Long term, we're better moving most of this to the frontend and persisting the theme in the background. Our mobile and desktop theme switcher being in two different parts of the app layout are whats preventing that at the moment. But I have plans for consolidating and improving the navigation soon ™️