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

Improve color selection and display #5057

Merged
merged 20 commits into from
Jan 6, 2024

Conversation

Nerixyz
Copy link
Contributor

@Nerixyz Nerixyz commented Dec 30, 2023

This PR improves color selection and display by doing the following changes:

  • A checkerboard is displayed where the color isn't opaque
  • Switched to saturation-brightness canvas
  • Added an alpha slider
  • The CSS input now shows alpha and accepts #rgb, #rrggbb, and #rrggbbaa (this is the CSS one, not the Qt one)
Before After
firefox_2023-12-30_16-29-55 chatterino_2023-12-30_16-27-01
firefox_2023-12-30_16-31-08 firefox_2023-12-30_16-28-57
same for users and badges

I'm still not sure about the rounding of the widgets and the circle selector, so feedback is welcome!

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 47. Check the log or trigger a new build to see more.

src/widgets/dialogs/ColorPickerDialog.cpp Show resolved Hide resolved
src/widgets/dialogs/ColorPickerDialog.cpp Show resolved Hide resolved
src/widgets/dialogs/ColorPickerDialog.cpp Show resolved Hide resolved
src/widgets/dialogs/ColorPickerDialog.cpp Show resolved Hide resolved
src/widgets/dialogs/ColorPickerDialog.cpp Outdated Show resolved Hide resolved
src/widgets/helper/color/ColorDetails.cpp Outdated Show resolved Hide resolved
src/widgets/helper/color/ColorDetails.cpp Outdated Show resolved Hide resolved
src/widgets/helper/color/ColorSelect.cpp Outdated Show resolved Hide resolved
src/widgets/helper/color/ColorSelect.hpp Outdated Show resolved Hide resolved
src/widgets/helper/color/ColorSelect.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 26. Check the log or trigger a new build to see more.

src/widgets/dialogs/ColorPickerDialog.cpp Show resolved Hide resolved
src/widgets/dialogs/ColorPickerDialog.cpp Show resolved Hide resolved
src/widgets/helper/color/ColorButton.cpp Show resolved Hide resolved
src/widgets/helper/color/ColorButton.cpp Show resolved Hide resolved
src/widgets/helper/color/ColorSelect.cpp Outdated Show resolved Hide resolved
src/widgets/helper/color/SBCanvas.cpp Show resolved Hide resolved
src/widgets/helper/color/SBCanvas.cpp Show resolved Hide resolved
src/widgets/helper/color/SBCanvas.cpp Show resolved Hide resolved
src/widgets/helper/color/SBCanvas.cpp Show resolved Hide resolved
src/widgets/helper/color/SBCanvas.cpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/widgets/helper/color/SBCanvas.cpp Show resolved Hide resolved
please drop support now
@pajlada
Copy link
Member

pajlada commented Dec 30, 2023

Some quick feedback

  1. At the current size, It looks like we should have room for at least one more row of recently used colors. Maybe we should add more recently used colors, or we should reduce the height of the dialog overall
  2. I don't mind the roundedness of the recently used/default colors/selected color blobs
  3. I don't mind the roundedness of the hue or alpha sliders
  4. I like the new color widget thing in the highlight dialog
  5. I miss the "Selected" label for the current color. I think it would be nice to have a similar box under it/next to it with the old color
  6. The circle cursor would be good if it had a solid color of the actually selected color (e.g. how it's done here: https://coloris.js.org/examples.html). I understand that might be a bit more challenging to implement, so I'm fine with using a simple + cursor too

@Nerixyz
Copy link
Contributor Author

Nerixyz commented Dec 31, 2023

At the current size, It looks like we should have room for at least one more row of recently used colors. Maybe we should add more recently used colors, or we should reduce the height of the dialog overall

The recent colors are just the colors of the message and user highlights. So if you have more entries with different colors, you'll have more recently used colors.

  1. I miss the "Selected" label for the current color. I think it would be nice to have a similar box under it/next to it with the old color

Added.

  1. The circle cursor would be good if it had a solid color of the actually selected color (e.g. how it's done here: coloris.js.org/examples.html). I understand that might be a bit more challenging to implement, so I'm fine with using a simple + cursor too

I added the opaque color, but it doesn't really make much of a difference. At least in the alpha slider, I think it's better to be transparent. Doing it the way it's done on the web isn't as easy, as we need to add another mask when painting the checkerboard (which seems overkill).

@pajlada
Copy link
Member

pajlada commented Dec 31, 2023

Can revert the circle opaque color in that case, it was probably fine

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

If it's not the biggest hassle, I'd like to get rid of the ColorSelect "widget" (and maybe even the ColorDetails) and just add them straight to the ColorPickerDialog - going through the anatomy of the dialog has been a few too many classes deep imo, maybe that will make it a bit simpler

src/widgets/helper/color/ColorSelect.hpp Outdated Show resolved Hide resolved
src/widgets/helper/color/ColorSelect.hpp Outdated Show resolved Hide resolved
src/widgets/helper/color/SBCanvas.hpp Show resolved Hide resolved
src/widgets/helper/color/SBCanvas.hpp Outdated Show resolved Hide resolved
src/widgets/helper/color/ColorDetails.cpp Outdated Show resolved Hide resolved
src/widgets/helper/color/ColorDetails.hpp Outdated Show resolved Hide resolved
src/widgets/helper/color/ColorSelect.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/widgets/dialogs/ColorPickerDialog.cpp Show resolved Hide resolved
src/widgets/dialogs/ColorPickerDialog.cpp Show resolved Hide resolved
src/widgets/helper/color/AlphaSlider.cpp Show resolved Hide resolved
src/widgets/helper/color/AlphaSlider.cpp Show resolved Hide resolved
src/widgets/helper/color/ColorInput.cpp Show resolved Hide resolved
src/widgets/helper/color/ColorInput.cpp Show resolved Hide resolved
src/widgets/helper/color/ColorInput.cpp Show resolved Hide resolved
src/widgets/helper/color/HueSlider.cpp Show resolved Hide resolved
src/widgets/helper/color/HueSlider.cpp Show resolved Hide resolved
src/widgets/helper/color/SBCanvas.cpp Show resolved Hide resolved
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Looks good to me! Just one quick review question & I'm happy to merge this in!

src/widgets/helper/color/ColorButton.cpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/widgets/dialogs/ColorPickerDialog.cpp Show resolved Hide resolved
src/widgets/dialogs/ColorPickerDialog.cpp Show resolved Hide resolved
src/widgets/dialogs/ColorPickerDialog.cpp Show resolved Hide resolved
src/widgets/dialogs/ColorPickerDialog.cpp Show resolved Hide resolved
src/widgets/helper/color/AlphaSlider.cpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/widgets/dialogs/ColorPickerDialog.cpp Show resolved Hide resolved
@pajlada pajlada changed the title refactor: Improve color selection and display Improve color selection and display Jan 6, 2024
@pajlada pajlada enabled auto-merge (squash) January 6, 2024 20:28
@pajlada pajlada merged commit 78a7ebb into Chatterino:master Jan 6, 2024
20 checks passed
@Nerixyz Nerixyz deleted the refactor/color-dialog-buttons branch January 6, 2024 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants