Skip to content
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

[BasicUI] Add preview color to Colorpicker widget #2873

Merged
merged 3 commits into from
Nov 18, 2024

Conversation

lolodomo
Copy link
Contributor

Also clenup class ColorpickerRenderer

Also clenup class ColorpickerRenderer

Signed-off-by: Laurent Garnier <[email protected]>
@lolodomo lolodomo added enhancement New feature or request basic ui Basic UI labels Nov 16, 2024
@lolodomo lolodomo requested a review from kaikreuzer as a code owner November 16, 2024 11:10
@lolodomo
Copy link
Contributor Author

The new rendering of the widget:

image

image

This is the same circle as in the Colortemperaturepicker widget.

@kaikreuzer
Copy link
Member

Why would we need the palette button here at all? It would feel more natural to me that I click on the colored circle if I want to change the color...

@lolodomo
Copy link
Contributor Author

lolodomo commented Nov 17, 2024

I was not sure but it was also what I was thinking ;)
Note that we should keep something consistent with the new Colortemperaturepicker widget.

Do you think I should just keep the new circle or rather keep only the rectangle button but with the current color inside and no icon ?

@kaikreuzer
Copy link
Member

Note that we should keep something consistent with the new Colortemperaturepicker widget.

Agreed. Having a look at it, I would actually say the same there: The picker button is redundant if the user could simply click on the colored circle.

Do you think I should just keep the new circle or rather keep only the rectangle button but with the current color inside and no icon ?

I don't have a strong opinion. The circle already "invites" the user to click on it, but we could also stick to the rectagular look of the buttons - this might be the more consistent option.

Do it also for the Colortemperaturepicker widget

Signed-off-by: Laurent Garnier <[email protected]>
@lolodomo
Copy link
Contributor Author

Kai, it is changed:

image

image

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice, I like it!
Thanks. 😄

@kaikreuzer kaikreuzer merged commit d5a0748 into openhab:main Nov 18, 2024
5 checks passed
@kaikreuzer kaikreuzer added this to the 4.3 milestone Nov 18, 2024
@lolodomo lolodomo deleted the basicui_color_preview branch November 18, 2024 09:14
lolodomo added a commit to lolodomo/openhab-docs that referenced this pull request Nov 24, 2024
Also include a small change of the Colorpicker element.

Related to openhab/openha-core#3891
Related to openhab/openhab-webui#2873

Signed-off-by: Laurent Garnier <[email protected]>
lolodomo added a commit to lolodomo/openhab-docs that referenced this pull request Nov 24, 2024
Also include a small change of the Colorpicker element.

Related to openhab/openhab-core#3891
Related to openhab/openhab-webui#2873

Signed-off-by: Laurent Garnier <[email protected]>
stefan-hoehn pushed a commit to openhab/openhab-docs that referenced this pull request Nov 29, 2024
* [sitemap] New Colortemperaturepicker element

Also include a small change of the Colorpicker element.

Related to openhab/openhab-core#3891
Related to openhab/openhab-webui#2873

Signed-off-by: Laurent Garnier <[email protected]>

* Review comments

Signed-off-by: Laurent Garnier <[email protected]>

---------

Signed-off-by: Laurent Garnier <[email protected]>
@felixschndr
Copy link

This looks very nice, thank you very much @lolodomo! Maybe we can built something like this for the other clients, looking at the android app @mueller-ma ? That's be great

@mueller-ma
Copy link
Member

I'm pretty sure that there was a preview like this when I merged @maniac103 PR that implements this new widget.

@maniac103
Copy link

maniac103 commented Dec 9, 2024

Yes, the Android app does it like that already [1] (displaying the color preview instead of the palette), with one tweak: if the light is off (brightness = 0) or the state is unknown, the palette is shown instead of the preview because a black preview looks somewhat ugly.

[1] In the beta, I did this for consistency when implementing the color temperature picker widget.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
basic ui Basic UI enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants