Feature/highlight only mode #17
Conversation
MarshallOfSound
force-pushed
the
feature/highlightOnlyMode
branch
from
5ca2779 to
09aaf2c
Compare
|
Do you happen to have a GPDMP dev build with this integrated? |
|
The implementation in GPMDP will have to change a "tiny" bit in order to enable this highlight only mode. Gimme a sec and I'll swing you a mac build |
|
@chrismou This is very naive implementation (I need to change the settings window to match the styling still) but the general idea is there. I'll probably have to change the defaults to be Google's orange. But here is the mac zip |
| key: "enableAll", | ||
| value: function enableAll() { | ||
| this.disable(); | ||
| document.querySelector('html').classList.add(CONSTANTS.CLASS_NAMESPACE); |
There was a problem hiding this comment.
How come the switch from body to html, out of interest?
Just wondering if this has the potential to be a breaking change, in the unlikely event that anyone's extending it and targetting .gmusic-theme.material on the body tag
There was a problem hiding this comment.
So that the light class can go on the body while still being a subclass to the main theme classs
(If that makes any sense) 👍
|
Aside from the question, all looks good to me on the implementation front. Clicking around and everything looks as expected, On an unrelated note, you're not actually providing the Google orange (#f16c00) as a colour option. :-P Also the settings menu header colour is slightly off (Photoshop is telling me it's #f17800) but I figure from what you said you know about that one. |
| #### `enable()` | ||
| Enables the current custom theme | ||
| Is a mirror of `enableAll`, present for backwards compatibility |
There was a problem hiding this comment.
I think "alias" is a more common term than "mirror" =/
|
Left a bunch of feedback. As you might guess, I'm opposed to calling it highlight since most theming libraries call themselves dark or light. Also, I think the creation of Therefore, it makes the most sense to make it an option in the same location: theme.updateTheme(type: 'highlight');If we want enforcement, then that can be done via asserts: // Define our constants
const THEME_TYPE_ALL = 'all';
const THEME_TYPE_HIGHLIGHT = 'highlight';
const SUPPORTED_THEME_TYPES = [THEME_TYPE_ALL, THEME_TYPE_HIGHLIGHT];
// Inside the `updateTheme` method
var assert = require('assert');
updateTheme: function (options) {
// If we received a theme, verify it's valid
if (options.type) {
assert.notEqual(SUPPORTED_THEME_TYPES.indexOf(options.type), -1,
'Expected `options.type` to be in ' + JSON.stringify(SUPPORTED_THEME_TYPES) + ' but it was not.');
}
// Update theme properties
this.BACK_PRIMARY = colorObject.backPrimary || this.BACK_PRIMARY;
// ...
this.TYPE = colorObject.type || this.TYPE;
this.redrawTheme();
}
// We can go even further and allow people to use these constants
GMusicTheme.TYPES = {
ALL: THEME_TYPE_ALL,
HIGHLIGHT: THEME_TYPE_HIGHLIGHT
};
theme.updateTheme(type: GMusicTheme.TYPES.HIGHLIGHT); |
|
@twolfson I agree with lots of your comments, but disagree with the idea that we should be calling it "light" and "dark". I specifically did not name them that because that is not what these methods do. You could use the Hence I labelled them after what they literally do as opposed to what they will be primarily used for. Does that make sense? |
|
Yea, it makes sense but I'm coming from an API design perspective. With a high level module like this, the goal should not be to describe the implementation to end users (e.g. in Instead, we communicate the behavior that the user expects/is looking for. Since the library is a theming library and the canonical language is light/dark, I'm pushing for light/dark so it can stay consistent with the canon. |
|
FWIW I'm not sure that applying API design principles to a library like this is entirely right, as this isn't an API - an API definitely shouldn't have methods that change layout, styles, etc. TBH, I'm not sure how I'd categorise this - a control module maybe? It's essentially a UI helper. It doesn't sit above the application, it's part of it. IMO, this is why your example works for But TBH, if we're talking about making decisions based on something that might happen in the future, I'm willing to go along with whatever call @MarshallOfSound makes on it, as long as proper thought's been given to how else this theme library could be used. If an app could use this library to create a dark theme by activating the "light theme" functionality and passing it different config values, then it probably shouldn't be called "light". |
|
I'm also supportive of whatever @MarshallOfSound decides on as long as it's discussed thoroughly =) @chrismou I think you might be getting caught up on semantics. When I refer to API design, I mean the interface that a programmer interacts with. This can be a method call or an HTTP call. That being said, you make a good point about a "dark highlight" and I'm more open to the naming as a result. It does irk me a little still though, maybe we could name it an "accent" color or similar. I should note that I would still prefer "highlight" (or whatever we call it) to be set via |
|
I looked up what the terms Google uses for Material design. It turns out they are primary, secondary, and accent. https://www.google.com/design/spec/style/color.html#color-color-schemes https://www.google.com/design/spec/style/color.html#color-text-background-colors Maybe we should reconsider our naming scheme to be more inline with Google. For example:
|
|
If we do reconsider our naming scheme for colors, let's do it in a separate PR to prevent overwhelming this one for reviewers x_x |
|
Also, if we do reconsider our naming scheme for colors, let's add a visual diff test suite so we can verify we have no visual regressions. I left some notes on this in #2 |
|
I was just referring to the comparison with gmusic.js and the point about it being high level. I got the feeling you were focussing more on the functionality that generated the dist/ folder files than the dist files themselves, but if I've misunderstood then I stand corrected 😊 Agreed on the naming scheme change. I've had to sift through the styles a couple of times to remind myself which is primary and which is secondary (I kept thinking primary was the accent) so yeah, a name change would be welcome if it removed any ambiguity. We should probably continue this discussion in an issue, as I suspect there'll be plenty of discussion and this chat will end up being buried when the PR gets through 👍 Same with the visual diff suite. The QAs at my work use one for testing ad generation changes, and they swear by it. Not sure which library they use though. |
MarshallOfSound
force-pushed
the
feature/highlightOnlyMode
branch
2 times, most recently
from
8004964 to
4ff0d83
Compare
|
I just updated the PR.
There are two other changes I want to do but not in this PR (I don't want breaking changes in this PR)
For now though, I think this PR should solely focus on implementing this feature not changing our API. |
MarshallOfSound
force-pushed
the
feature/highlightOnlyMode
branch
from
4ff0d83 to
84a3c5a
Compare
| @@ -1,3 +1,4 @@ | |||
| { | |||
| "extends": "airbnb/base", | |||
| "parser": "babel-eslint" | |||
There was a problem hiding this comment.
It seems that babel-eslint has slipped into this PR. Can we relocate it to a preceeding PR?
There was a problem hiding this comment.
It doesn't make sense for it to be in it's own PR as it literally does nothing other than add ES7 syntax support to eslint. We don't use ES7 until this commit so the history wouldn't make much sense?
There was a problem hiding this comment.
The purpose of breaking down PRs is typically to make the PRs smaller so there's less things for reviewers to consider when reviewing each PR.
Additionally, it makes sure that backwards compatibility doesn't break for library/build upgrades.
However, I think I meant to comment on plugins in Gruntfile rather than the .eslintrc =/
There was a problem hiding this comment.
In this case I think is is an exception as the addition of this and the plugins line in the gruntfile is too allow the new code to work. It would make sense from a historical standpoint to add dependencies that don't do anything in one PR and then start using them in a different PR?
I think this might be more of a debate into how to use git...
There was a problem hiding this comment.
Yea, I think my comment is mostly pedantic. We should be fine for a small project like this.
MarshallOfSound
force-pushed
the
feature/highlightOnlyMode
branch
from
aba4a24 to
6e442a4
Compare
|
@twolfson Ta da? 😄 |
|
|
||
| class GMusicTheme { | ||
| static TYPES = { | ||
| FULL: 'FULl', |
There was a problem hiding this comment.
Typo here, we want FULL notFULl =P
|
Looks great, almost at the finish line. I think we might be talking past each other again with respect to the
Then, we can keep it simple and only add/remove a single // At the top
const COMMON_CSS = fs.readFileSync(__dirname + '/../build/common.css', 'utf8');
const FULL_CSS = fs.readFileSync(__dirname + '/../build/full.css', 'utf8');
const HIGHLIGHT_CSS = fs.readFileSync(__dirname + '/../build/highlight.css', 'utf8');
// In _refreshStyleSheet
let styles = COMMON_CSS;
if (this.type === GMusicTheme.TYPES.FULL) {
styles += FULL_CSS;
} else if (this.type === GMusicTheme.TYPES.HIGHLIGHT_ONLY) {
styles += HIGHLIGHT_CSS;
}
this.styleElement.innerHTML = this._substituteColors(styles); |
| @@ -74,6 +96,7 @@ window.GMusicTheme = class GMusicTheme { | |||
| this.BACK_HIGHLIGHT = colorObject.backHighlight || this.BACK_HIGHLIGHT; | |||
| this.FORE_PRIMARY = colorObject.forePrimary || this.FORE_PRIMARY; | |||
| this.FORE_SECONDARY = colorObject.foreSecondary || this.FORE_SECONDARY; | |||
| this.type = colorObject.type || this.type; | |||
There was a problem hiding this comment.
We should probably add an assertion for colorObject.type to be kind to developers who got a typo (or miss the deprecation notice for accent when we introduce it):
static TYPES = {
FULL: 'FULL',
HIGHLIGHT_ONLY: 'HIGHLIGHT_ONLY',
};
static VALID_TYPES = [TYPES.FULL, TYPES.HIGHLIGHT_ONLY];
updateTheme(colorObject) {
if (colorObject.type !== undefined && GMusicTheme.VALID_TYPES.indexOf(colorObject.type) === -1) {
throw new Error(`\`updateTheme\` expected \`colorObject.type\` to be in ${JSON.stringify(GMusicTheme.VALID_TYPES)} ` +
`but it was "${colorObject.type}"`);
}There was a problem hiding this comment.
Oh, silly me. We could have used the object for an even simpler assert:
updateTheme(colorObject) {
if (colorObject.type !== undefined && GMusicTheme.TYPES[colorObject.type] === undefined) {
let validTypesStr = JSON.stringify(Object.keys(GMusicTheme.TYPES));
throw new Error(`\`updateTheme\` expected \`colorObject.type\` to be in ${validTypesStr} ` +
`but it was "${colorObject.type}"`);
}
17a6613 to
3e3d479
Compare
|
@twolfson Finally got around to getting back to this PR. I've fixed the things you mentioned and I have also changed the default "highlight / accent" color to be Googles orange as suggested by @chrismou I think the CSS file structure now is logical and the naming makes more sense that it did so 👍 Is this GTGN? 🌵 |
| @@ -1,3 +1,4 @@ | |||
| module.exports = { | |||
| CLASS_NAMESPACE: 'gmusic-theme', | |||
| CLASS_NAMESPACE_HIGHLIGHT: 'gmusic-theme-highlight', | |||
There was a problem hiding this comment.
If we are using concatenation, then we should no longer need this highlighting class (since there's nothing to override via HTML elements).
|
It looks like we buried a comment that hasn't been fully addressed =/ Going to see if I can find it... |
MarshallOfSound
force-pushed
the
feature/highlightOnlyMode
branch
from
3e3d479 to
1e8012b
Compare
|
Alright, so it looks like we agreed on updating To extend on them further, it looks like the majority of the lines in gmusic-theme.js/lib/highlight.css Lines 1 to 171 in 1e8012b Then, the concatenation from #17 (comment) should be usable: |
MarshallOfSound
force-pushed
the
feature/highlightOnlyMode
branch
from
1e8012b to
f2a91a7
Compare
|
@twolfson Finally clicked onto what you were saying this whole time 👍 I switched up your |
| @@ -56,7 +56,7 @@ included will not be changed from the defaults. `enabled` is set to false by de | |||
|
|
|||
| ### States | |||
|
|
|||
| #### `enable()` | |||
| #### `enable` | |||
There was a problem hiding this comment.
I think we accidentally deleted the parentheses here. We should prob keep them to be consistent with the rest of the methods
|
Looks good. Left a few pedantic comments. Feel free to ship with or without them |
|
Also, I know we have been doing a lot of patch releases but this should prob be a minor release =P |
MarshallOfSound
force-pushed
the
feature/highlightOnlyMode
branch
from
f2a91a7 to
0641f32
Compare
|
Fixed up a couple of pedantics. Left the default case as it is a linting thing 🌵 I'll land this in a minor once travis finishes |
|
👍 |
Feature/highlight only mode
Implements what some call a "light" mode but for readabilities sake I am called it "highlight only" mode because that is what it actually does.
It disables all rules that aren't the highlight color and adds in a couple of extra ones to get the right look and feel.
As an example here is what GPM looks like with this mode active
The original "dark style" mode is unaffected by these changes and the old
enable()method exists and works the exact same way it used to work/cc @gmusic-utils/gmusic-theme-js @gmusic-utils/gmusic-js