feat: themes, sortie boss name, cookies #172
Conversation
TobiTenno
force-pushed
the
themes-cookies
branch
from
f03a9cb to
c6ef940
Compare
assets/js/main.js
Outdated
| @@ -22,7 +22,7 @@ const cleanupNotifiedIds = () => { | |||
| .concat([worldState.cetusCycle.id]) | |||
| .concat([worldState.voidTrader.id]) | |||
| .concat(worldState.persistentEnemies.map(enemy => enemy.pid)); | |||
| const notifiedIds = JSON.parse(localStorage.getItem('notifiedIds') || '[]'); | |||
| const notifiedIds = JSON.parse(localStorage.notifiedIds || '[]'); | |||
There was a problem hiding this comment.
Is there a particular reason we're switching off of the built-in function calls?
There was a problem hiding this comment.
these are a bit more straight-forward when we know the names for certain
There was a problem hiding this comment.
In case of changes to the Storage object, I'd prefer to stick with the function calls, especially with localStorage.mode.
| Cookies.set(component, value, {expires: 365}); | ||
| const value = localStorage.getItem(component); | ||
| if (typeof value === 'undefined' || value === null) { | ||
| localStorage.setItem(component, defValue || 'true'); |
There was a problem hiding this comment.
Not relevant to the review, but is there a reason for using the string literals instead of booleans?
There was a problem hiding this comment.
localStorage stores strings
assets/less/main.less
Outdated
|
|
||
| /* Fish info table */ | ||
| .fish-info { | ||
| border: 2px solid #1a242f !important; |
There was a problem hiding this comment.
Could any of these !importants be removed now that there is a more specific selector?
There was a problem hiding this comment.
doesn't really matter, but i can try and check
| @@ -24,72 +24,75 @@ | |||
| <script src="https://unpkg.com/draggabilly@2/dist/draggabilly.pkgd.min.js"></script> | |||
| <!-- Cookies CDN --> | |||
| <script src="https://cdnjs.cloudflare.com/ajax/libs/js-cookie/2.1.4/js.cookie.min.js"></script> | |||
| <!-- Cookie Consent --> | |||
| <link rel="stylesheet" type="text/css" href="https://cdnjs.cloudflare.com/ajax/libs/cookieconsent2/3.0.3/cookieconsent.min.css" /> | |||
| <script src="https://cdnjs.cloudflare.com/ajax/libs/cookieconsent2/3.0.3/cookieconsent.min.js"></script> | |||
There was a problem hiding this comment.
🎉 Incoming Lighthouse improvement
views/partials/dependencies.hbs
Outdated
| themecustom.attr('href', "/css/main.retro.css?v={{lookup sums 'css/main.retro.css'}}"); | ||
| theme.toggleClass('retro'); | ||
| $('html').removeClass(); | ||
| $('html').addClass('retro'); |
There was a problem hiding this comment.
Would simply $('html').attr('class', 'retro') do?
views/partials/themes.hbs
Outdated
| Cookies.set('mode', 'day', {expires: 365}); | ||
| html.removeClass(localStorage.mode); | ||
| localStorage.mode = 'day'; | ||
| html.attr('data-theme', 'day'); |
There was a problem hiding this comment.
Do we use this attribute for anything?
There was a problem hiding this comment.
no, meant to drop it back off
views/partials/themes.hbs
Outdated
| theme.removeClass(); | ||
| Cookies.set('mode', 'day', {expires: 365}); | ||
| html.removeClass(localStorage.mode); |
There was a problem hiding this comment.
This block is really similar to the one above. Would it be possible to combine the two? Something along the lines of just setting themeName = 'day' whenever reset || theme.hasClass(themeName)?
There was a problem hiding this comment.
uh, if class has themename, it falls into this this block already
There was a problem hiding this comment.
Ugh, oops, I forgot the ! before hasClass.
|
oh, i haven't merged yet 🤦♂️ |
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Warframe Hub Pull Request
Summary (short):
Closes #155 #167
Initial change for #166, delete checking code in a month
Description:
Makes sortie timer earlier in DOM compared to rest of sortie info
Shift dependence on cookies to localStorage
Use themes over html class, eventually refactor boostrap primary configs to
.lessso we can "theme" them, nest them inside of theme classes, easierFixes issue (include link):
#155 #166 #167
Mockups, screenshots, evidence:
See deployment
(Check one)