search

yjs / y-prosemirror

ProseMirror editor binding for Yjs
https://demos.yjs.dev/prosemirror/prosemirror.html
MIT License
328 stars 115 forks source link

feat: implement generic color capabilities #154

Open troglodytto opened 2 months ago

troglodytto commented 2 months ago
troglodytto commented 2 months ago

@dmonad Just bringing this to attention

piyush-kroolo commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.