-
Notifications
You must be signed in to change notification settings - Fork 75
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what caused the options panel lag when this was first merged but since this PR only updates the CSS that bug should still be here.
Using a Set
would be simpler and less error prone than a bit field. The banner board list uses this approach.
I'm not a fan of the stacked banners. Instead of changing the default look, why not design it to be easy to style the way you like with custom CSS?
switch (oldNowPlaying) { | ||
case "false": | ||
case "none": | ||
localStorage.setItem("nowPlaying", "" + (0 ^ 0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
localStorage.setItem("nowPlaying", "" + (0 ^ 0)); | |
localStorage.setItem("nowPlaying", "0"); |
This is just confusing. Add a comment if you think the reader won't understand bit flags. Same with the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's unnecessary for that part, but I think particularly in specs.ts it makes it clearer how to add new options.
break | ||
let matched = false; | ||
let s: string[] = []; | ||
for (const i of enabled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post name replacement should only happen when r/a/dio matches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. It currently happens if eden is the only enabled station, so I see no reason to exclude it now.
I don't think that actually was lag, it was odd behaviour that just seemed like it. Should be fixed.
Maybe. I'll admit I didn't look at how the board list worked before writing this, but logic mistakes aside I still quite like this way.
Things already get quite tight with both stations enabled. If there were 3 or more enabled, it would be a cramped mess unless you had a massive monitor. The default should be usable. The new html layout does make it easy to put it back inline with custom css if you prefer it that way. |
Seems to be fixed. Merging. |
Had the wrong part of the overlay stickied last time. This should work properly.