search

yjs / y-prosemirror

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

Replace transparency from color if present #76

Closed Flamenco closed 2 years ago

dmonad commented 2 years ago

Thanks for offering an improvement @Flamenco.

However, I don't think that this is the right approach. y-prosemirror accepts a 6-character long hex color without transparency. There are many more edge cases that I'd have to cover if y-prosemirror accepted any kind of color (3-character hex colors, HSL colors, and so on..). It is better to keep this minimal. If you have transparent colors stored, you could transform them before you supply them to y-prosemirror.

Flamenco commented 2 years ago

The problem is when my color editor sets the color from tiptap's collab cursor plugin, the cursor shows but not the highlighted area. This was traced to y-prosemirror not checking the format. In the end a regular expression and other color formats would be best.

I'd call this a hotfix.

Flamenco commented 2 years ago

@dmonad I think cleaning up this edgecase is valid. Just because you want to support other in the future is not a reason to let your code generate garbage.

If you noticed, i did not allow the opacity to go through for that very reason, and used your appended value.

Flamenco commented 2 years ago

At the very least, your code should validate the input, output and warning message, and your strict color format should be clearly documented.

dmonad commented 2 years ago

The cursor plugin in y-prosemirror does not support transparent colors. Supporting other color formats is out of the scope of this project. I don't want to pull in another library just to do color transformations (aside from that, I'd need to maintain many more edge cases because of this choice). y-prosemirror only supports 6-digit hex colors which is a fair restriction. The user of this library can do the color transformation. Though it really would be nice if there was an error message when the user supplies an invalid color format. Is that something you'd like to work on? Happy to merge a PR that logs a warning and maybe documents the feature better.

Flamenco commented 2 years ago

@dmonad I will resubmit a PR, and emit a warning, but you are still opposed to truncating the hex transparency with 8 digit hex color?

You just want to let the code make it a 10 digit hex instead? I really don't understand the logic in that.

Flamenco commented 2 years ago

Warning only commit: https://github.com/yjs/y-prosemirror/pull/78