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

feat: implement generic color capabilities #154

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

troglodytto
Copy link

@troglodytto troglodytto commented May 31, 2024

  • implement color utils to allow the usage of multiple kinds of colors instead of JUST Hex
  • now supports Hex, RGB, HSL
  • includes test cases for 3 different formats of colors

- implement color utils to allow the usage of multiple kinds of colors instead of *JUST* RGB
- now supports Hex, RGB, HSL
- includes test cases for 3 different formats of colors
@troglodytto
Copy link
Author

@dmonad Just bringing this to attention

@piyush-kroolo
Copy link

I too would like to see this @dmonad. Right now, I'm having to use a hex color, but I'd like to use an HSL value so that I can decide the brightness of the colors depending on the theme that the user is on, and this feature would make it easier to implement without having to fall back to first converting the HSL values to Hex (by either implementing the conversion code ourselves or relying on third party libraries like tinycolor etc.) and then using that hex as the input.

@dmonad
Copy link
Member

dmonad commented Jun 10, 2024

Thank you for opening a PR!

I appreciate that the current system is simple. Users colors are always represented as hex colors in Awareness. Currently, most editor bindings builts with Yjs only support hex colors. Adding full support for the different color representations might be complicated across the whole ecosystem. Other Yjs extensions will probably throw errors when I merge this and another extension uses HSL colors.

Although my approach is definitely not perfect - I know that.

I'm currently not sure whether I want to support other color representations. Ultimately, it would be up to me to maintain this, fix bugs, and understand how it works. That is currently not in my interest. I hope you understand.

I'll leave this PR open for now.

@troglodytto
Copy link
Author

Thanks for your input @dmonad .

Yes, We can keep this PR open for now.

Can you please let me know which kind of potential errors other extensions might throw?

I've tried to keep this as backwards compatible as possible.

But I'll try finding solutions to make it fully compatible with other extensions in the meantime, basis on the potential errors that you mention.

PS. For Anyone else reading this, an alternative approach would be to use something like https://github.com/bgrins/TinyColor and convert your colors to a Hex string (specifically a 6 digit Hex string, Not a 3,4 or 8 digit one)

@dmonad
Copy link
Member

dmonad commented Jun 10, 2024

It's just that other extensions don't understand hsl. Supporting hsl is like opening Pandora's box: eventually we'd need to support all formats in all extensions. These formats basically do the same thing, so I'm not sure I want to open that box.

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.

3 participants