2

I have a function that sets the color theme of my site when clicked, but when I reload the page or go to a new page, it reverts back to the default theme. There's a line of my code that is supposed to get the previous theme from localStorage and apply it to the new one, but I think why it isn't working is that there isn't actually any code that saves the theme to Storage. Either that, or localStorage resets when the page is reloaded/changed. Either way, I'm not sure how to fix it.

This is my code:

<a class="switch-light-purple color-palette" id="color-palette" onclick="setTheme('theme-light-purple')">
<script>
  // function to set a given theme/color-scheme
  function setTheme(themeName) {
    localStorage.setItem('theme', themeName);
    document.documentElement.className = themeName;
  }

  // Immediately invoked function to set the theme on initial load
  (function () {
    if (localStorage.getItem('theme') === 'theme-light-purple') {
      setTheme('theme-light-purple');
    }
  })();
</script>
</a>

<a class="switch-dark-blue color-palette" id="color-palette" onclick="setTheme('theme-dark-blue')">
<script>
  // function to set a given theme/color-scheme
  function setTheme(themeName) {
    localStorage.setItem('theme', themeName);
    document.documentElement.className = themeName;
  }

  // Immediately invoked function to set the theme on initial load
  (function () {
    if (localStorage.getItem('theme') === 'theme-dark-blue') {
      setTheme('theme-dark-blue');
    }
  })();
</script>
</a>
painotpi
  • 6,894
  • 1
  • 37
  • 70
Alexander
  • 131
  • 9
  • Any reason to nest `script` tags within `anchor` tags? – painotpi May 17 '21 at 07:58
  • Why are you repeating your script? Why compare `localStorage.getItem('theme')` against things? Just do `if(localStorage.getItem('theme')){ setTheme(localStorage.getItem('theme')); }`. Repeating the same `id` is invalid in HTML. Inline event handlers like `onclick` are [not recommended](/q/11737873/4642212). They are an [obsolete, hard-to-maintain and unintuitive](/a/43459991/4642212) way of registering events. Always [use `addEventListener`](//developer.mozilla.org/en-US/docs/Learn/JavaScript/Building_blocks/Events#inline_event_handlers_%E2%80%94_dont_use_these) instead. – Sebastian Simon May 17 '21 at 07:59
  • I'm relatively new to Javascript. Do I not have to do that? – Alexander May 17 '21 at 07:59
  • Thanks, I'll try that out. I actually have more of these anchors (there are 12 buttons in total) but I didn't want to write out 12 near-copies of code. That's why it isn't written as a simple if/else statement. – Alexander May 17 '21 at 08:03
  • For the event listener - do I just replace onclick with addEventListener? Or is there more I have to do to replace the function? – Alexander May 17 '21 at 08:04
  • @EmrysMayell I've added an answer to help clear some of this out for you, hope it helps. – painotpi May 17 '21 at 08:36

1 Answers1

1

Separation of concerns:

Keep your html, css, and js in their respective files ALWAYS, there's no substitution for this.

So your index.html should look something like this (assuming index.html, style.css, and main.js are all in the same folder).

<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta http-equiv="X-UA-Compatible" content="IE=edge" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <title>Document</title>
    <link rel="stylesheet" href="style.css" />
  </head>
  <body>
    <div class="parent-container">
      <a data-theme="theme-dark-blue" class="switch-dark-blue color-palette" id="color-palette">Dark Blue </a>
      <a data-theme="theme-light-purple" class="switch-light-purple color-palette" id="color-palette2">Light Purple</a>
    </div>
    <script src="main.js"></script>
  </body>
</html>

Your CSS will be in a file called style.css,

And your JS,

// This waits for the DOM to complete loading
document.addEventListener('DOMContentLoaded', function() {
  // all your code goes inside this.
});

Also, as @Sebastian Simon pointed out in the comments,

Repeating the same id is invalid in HTML. Inline event handlers like onclick are not recommended. They are an obsolete, hard-to-maintain and unintuitive way of registering events. Always use addEventListener instead.

Unique IDs:

You could change the id of one of your anchor tags to color-palette2 for instance, but since you have a lot of different themes you could pick names that are more self-explanatory. But, as long as your ids are unique (1 unique id per page), it's fine.

Event Listeners:

Use .addEventListener to add events to your DOM nodes. I've done this in the code snippet I've shared. It's very easy once you get used to it.

I've also used data attributes to pass your theme in a more "dynamic" way to your logic. StackOverflow snippets don't support localStorage hence I've commented that line out.

function setTheme() {
  let theme;
  let savedTheme = localStorage.getItem('theme')
  
  // check if localstorage already has a theme defined
  // if it has, then set theme variable to it
  // otherwise, get the theme from the data-theme attribute
  // of the current anchor tag clicked represented by this.
  theme = savedTheme || this.dataset.theme
  
  let div = document.querySelector('div.parent-container')
  
  // I'm adding this to a div
  // you can add it to the body tag.
  div.classList.add(theme)
  
  
  // this code can be better but the idea is 
  // to remove all other themes and keep only the intended one.
  if (theme === 'theme-dark-blue') {
    div.classList.remove('theme-light-purple')
  } else {
    div.classList.remove('theme-dark-blue')
  }
  
  console.log(`Set theme to ${theme}`)
}

let palettes = document.querySelectorAll('.color-palette')
palettes.forEach(function(p) {
  p.addEventListener('click', setTheme)
})
a {
  cursor: pointer;
  text-decoration: underline;
  color: blue;
}

.switch-light-purple {
  color: purple;
  margin: 0 15px;
}
<div class="parent-container">
  <a data-theme="theme-dark-blue" class="switch-dark-blue color-palette" id="color-palette">Dark Blue </a>

  <a data-theme="theme-light-purple" class="switch-light-purple color-palette" id="color-palette2">Light Purple</a>
</div>

PS: the code snippet won't work since StackOverflow does not support localStorage.

painotpi
  • 6,894
  • 1
  • 37
  • 70