-
Notifications
You must be signed in to change notification settings - Fork 279
fix(LIGHT): show color picker based on feature check #488
Conversation
This is a behavior change since now even if the user doesn't define sliders and light supports color change then picker will still show up on long press. We maybe should leave support for "item.colorpicker" to be able to override it to "false" but not sure if it's worth it.
I believe, we discussed in #313 that we wanted to make an automatic history/brightness/temperature/color popup for this. However, I actually like the slim looks of the light sliders on the tile. It's just not usable for me, since it turns on the light on full brightness before I even get a chance to adjust the brightness (see #312). So, in general, I believe we should revisit the whole light slider thing. Either by a popup. Or by changed long-press behavior plus automatic slider configuration. I just wonder where we'd put a history graph then, if we want it to be available for lights. That being said, more automatic configurations are a good thing. Still, the major point TileBoard makes is its being customizable. So, I'd prefer to allow overriding automatic choices, here with Do you maybe see a way to have overrideable default choices? E.g., don't check TileBoard/scripts/controllers/main.js Line 2152 in e796af5
with
Would that make sense? We could then easily add lots of default options. And make all of them overrideable... |
I've done as you've suggested with the exception that I'm not merging defaults dynamically from |
Very nice new capacity! I'll need some time to check it out. But I'm excited! |
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.
Looks great, just a few comments that might help improve even more.
@@ -269,3 +276,25 @@ export const DEFAULT_VOLUME_SLIDER_OPTIONS = { | |||
field: 'volume_level', | |||
}, | |||
}; | |||
|
|||
export const TILE_DEFAULTS = { |
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.
Should we explain somewhere in the docs that the following will now be possible in config.js
?
TILE_DEFAULTS[TYPES.LIGHT].colorpicker = false;
Maybe be even add a line to the example:
TILE_DEFAULTS = angular.merge(TILE_DEFAULTS, {
// override defaults here, e.g.:
[TYPES.LIGHT]: {
colorpicker: false
}
});
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.
We should but I haven't figured out a way to state that in a generic way. Maybe we should wait until we have all tiles covered that way because now the instructions would have to be rather specific.
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.
Also fine with me, that would give us time to sort out last issues with this new scheme before people start to use it. I'll certainly help find those issues, because I'll switch my whole setup to use the defaults.
We just shouldn't forget...
@@ -269,3 +276,25 @@ export const DEFAULT_VOLUME_SLIDER_OPTIONS = { | |||
field: 'volume_level', | |||
}, | |||
}; | |||
|
|||
export const TILE_DEFAULTS = { |
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.
Also fine with me, that would give us time to sort out last issues with this new scheme before people start to use it. I'll certainly help find those issues, because I'll switch my whole setup to use the defaults.
We just shouldn't forget...
No further comments. This is a great improvement! |
This is a behavior change since now even if the user doesn't define
sliders and the light supports color change, then picker will still show up
on long press.
We maybe should leave support for "item.colorpicker" to be able to
override it to "false" but not sure if it's worth it.
@akloeckner