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

(WIP) Color Picker #1620

Closed
wants to merge 2 commits into from

Conversation

G-Ambatte
Copy link
Collaborator

This draft PR is an initial pass at adding a color picker tool to the Text and Style tabs. As of this commit, the color picker is functional and updates the text in the editor:

  • If text is selected, any color values in the selection are replaced with the picked color (identified as "#" followed by 3, 4, 6, or 8 hexadecimal values).
  • If no text is selected, the hex value of the picked color is injected at the current cursor position.

Once completed, this PR will resolve issue #1422.

@Gazook89
Copy link
Collaborator

Gazook89 commented Sep 5, 2021

Out of curiosity, why not use codemirror-colorpicker? It seems to be well integrated with codemirror.

I think it would be handy to have the inline color picker which opens when clicking on a :before element, such as the below (done with just browser inspector):

image

@calculuschild
Copy link
Member

calculuschild commented Sep 5, 2021

@Gazook89 Is that something we can integrate with our v3 markdown or is it only compatible with the editor in CSS mode?

@Gazook89
Copy link
Collaborator

Gazook89 commented Sep 6, 2021

@calculuschild i am not sure, in regards to codemirror-colorpicker.

With react-color, I tried building on this PR to add an element in front of hexadecimal color codes to get started, but didn't get very far. I was using the functions in markdown.js that convert the {{ }} into divs as a guide, but really didn't know what i was doing after a point.

@G-Ambatte
Copy link
Collaborator Author

Out of curiosity, why not use codemirror-colorpicker? It seems to be well integrated with codemirror.

I tried using codemirror-colorpicker but had issues trying to get it to play nicely with React - I found it easier to get react-color to play nicely with CodeMirror.

I think it would be handy to have the inline color picker which opens when clicking on a :before element, such as the below (done with just browser inspector):

As do I; I don't expect the current iteration to be the final product. However if someone else wants to run with this feature, either using this work as a starting point or ignoring it completely, I would not be upset at all.

@calculuschild calculuschild added the P3 - low priority Obscure tweak or fix for a single user label Oct 7, 2021
@calculuschild calculuschild changed the base branch from master to PRODUCTION November 4, 2021 02:23
@calculuschild calculuschild changed the base branch from PRODUCTION to master November 4, 2021 02:23
@calculuschild
Copy link
Member

I've finally had a chance to play around with this. I do like the idea of it, but here are my thoughts for moving forward:

  • Having icons inline such as using codemirror-colorpicker would be especially handy, as agreed above. I think it's worth another visit to try getting that to work.
  • Perhaps additionally, we can keep the little button, but alongside the undo/redo buttons as another "tool" separate from the snippets.
  • I don't like having to select text for the color picker to do anything. Instead I think it would be easier if selecting any color just injected that hex right at the cursor, and then subsequent selections if you haven't moved the cursor would just overwrite that hex again.

@Gazook89
Copy link
Collaborator

Just a thought while I was doing the code editor page numbers: this might be a good use of cm.setBookmark() to add an inline widget/node. Or maybe addWidget().

I don't have anything to add beyond that. 🤷

@calculuschild
Copy link
Member

This might be overtaken by #1870, although this is a good reference for that.

Any opposed if we close this down?

@calculuschild calculuschild added the 🔍 R1 - Reviewed - Needs Discussion 💬 PR not okayed yet; needs reevaluation or closure label Dec 11, 2021
@ericscheid
Copy link
Collaborator

I support the general principle of doing-the-work / keeping-notes in the one issue, so a maybe-yes to closing.

That said, would be sad if progress on this one feature was blocked by slower progress on a much larger effort. Could we re-point/revise this issue to be compatible with #1870 and marking it as Blocked? Are there things in #1870 that would be a major blocker to this?

@calculuschild
Copy link
Member

#1870 also proposes a whole different mechanism for summoning the color wheel, so it's built upon a different code structure that this PR isn't really compatible with. In other words, we could spend the effort to make this PR work, but it would just be torn town and rebuilt later once #1870 is underway, which is in turn waiting for "themes".

@jeddai
Copy link
Collaborator

jeddai commented Dec 14, 2021

@G-Ambatte

I tried using codemirror-colorpicker but had issues trying to get it to play nicely with React - I found it easier to get react-color to play nicely with CodeMirror.

I have some tricks for handling CodeMirror+React issues from when I was setting up the snippet options proof of concept. Was it issues with elements being redrawn on changes?

I actually read all the comments and yeah a lot would be rewritten for #1870, though it might be good on a standalone basis for the style tab.

@calculuschild
Copy link
Member

Closing. This needs further discussion on the underlying issue to make sure how/whether this needs to be handled going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 - low priority Obscure tweak or fix for a single user 🔍 R1 - Reviewed - Needs Discussion 💬 PR not okayed yet; needs reevaluation or closure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants