-
-
Notifications
You must be signed in to change notification settings - Fork 288
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dashboard color picker #945
Dashboard color picker #945
Conversation
Codecov Report
@@ Coverage Diff @@
## master #945 +/- ##
=======================================
Coverage 94.65% 94.66%
=======================================
Files 494 495 +1
Lines 6552 6564 +12
=======================================
+ Hits 6202 6214 +12
Misses 350 350
Continue to review full report at Codecov.
|
b3e1d9a
to
63edb93
Compare
Ok, not really fan of the "auto-close" setup, I'll work more on this. |
63edb93
to
be77d59
Compare
Thanks for the PR ! It's something that will help lots of people 😃 I have a question on this, in which format do you get/save the color ? We had a huuuuge debate on this during the v4 specification. The topic is partly here: https://community.gladysassistant.com/t/parlons-de-gladys-v4/3578/179?u=pierre-gilles I think we were talking about using the HSL format as "source of truth" format in Gladys, and then each service would convert the HSL to the color format he needs. |
I simply convert rgb hex to int, as feature consumes int. |
Are we sure that storing colors as integer is not losing information ? For example, Philips Hue is handling 16 millions colors. Are we able to represent that in integer ? We want that the data format we choose in Gladys to represent colors is able to handle all kind of lights and we don't want to lose information during those conversions. Feature is not necessarily an integer, it can be a string too with Too me this is an important choice, I remember that in the v3 we chose the integer type just because it was easier for us as developers, but people were complaining that colors in Gladys were not the same as colors displayed on their lamp. We need to think about this end-to-end with integrations. |
You're right, and I think we should have different type of color feature. But according to docs, max INT value (32 bits) is 2.147.483.647. So int can be enough to handle 16.000.000 colors. For more, in every cases, we'll have to check all current services to change it (to use new HSL / RBG / HUE color feature, or to handle string value). Ok topic is still open. |
front/src/components/boxs/device-in-room/device-features/ColorDeviceFeature.jsx
Show resolved
Hide resolved
Let's take real world examples! On the Philips Hue documentation, I see that:
But this is theory, on the Philips Hue Node.js implementation documentation, I can see that: I know this can looks like really precise stuff, but I remember that in the v3 people were complaining that Gladys color picker was wrong and not really usable (example: white is too white, how to set the warm white?), so I don't want to make the same mistake ! Maybe this RGB / Int implementation is enough, I just want to make sure this is working as expected :) Did you test this PR with real lights ? |
Wow... |
I will test your PR on philips devices |
Thank you for your feedbacks, I'm going to check for the color picker behavior (1st picture). |
be77d59
to
709c334
Compare
Ok I change current version and add Tasmota / PhilipsHue color management. Can you please try again? |
While waiting for more feedbacks on this, I put this PR as draft so it's more clear for me in the PR list. @VonOx let us know when you tested this :) |
Hi, |
@atrovato , it's ok with my philips devices. Colors set in dashboard seems to match real lamp color ( just a feeling ) |
270d06f
to
a4dc466
Compare
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.
Thanks for this PR, I really like the color picker and the integration: it works perfectly 👌
Good job! :)
I tested it with MQTT and it worked very smoothly: https://streamable.com/siqiyd
Just one feedback, I have a weird TypeError in the UI:
It's the only bug I had. Otherwise I'm all in to merge this!
b233ba5
to
5f0ee87
Compare
Oww... the click listener was not well removed, even if the component is unmount. |
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.
A really great PR as always, I'm happy to merge it 🥳
Thanks for the changes!
* Dashboard color picker * Tasmota color * PhilipsHue color * Fix color picker removeEventListener
Color-picker on dashboard.
Add auto-close when mouse leave.
Set picker as absolute div to avoid scrollbar.
Next steps: