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

add support for document color #45

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

Conversation

griffin-stewie
Copy link
Contributor

I added support for named document colors.

Steps to get name:

  1. Try to get name from document color
  2. Try to get alias color name
  3. Finally, use the color classifier.

I added to edit a name of document color inside colorNameChanged as well.

Consideration

My understanding is there was no "named document color" feature on Sketch itself when Prism came out. However now Sketch has "named document colors" since Sketch v53. So how to manage colors inside Prism is time to change MSColor based to MSColorAsset base.

It could be significant change so I didn't change now, but we may consider to change design.

Related Issue

#43

@custa1200
Copy link

Any chance these pull request could be actioned?

@custa1200
Copy link

One issue with this @griffin-stewie is that if I have 2 named colors that happen to have the same hex value, the second color will be named the same as the first one. when using named colors there is always the chance of having multiple versions of the same color with the same name.

@custa1200
Copy link

As an example the image has a transparent color which is White with 0 alpha, it is also naming the White at the bottom with 100 alpha Transparent. There is 2 greys as you can see too, the second one is named "Selected" in the named color list but it gets assigned Grey a second time.

CleanShot 2020-05-18 at 10 37 00@2x

@griffin-stewie
Copy link
Contributor Author

@custa1200
Is what you're calling "Named Color" the content of this link? or what you named them on the Prism's artboard?

@griffin-stewie
Copy link
Contributor Author

griffin-stewie commented May 18, 2020

@custa1200
I'm starting to get it.

As an example the image has a transparent color which is White with 0 alpha, it is also naming the White at the bottom with 100 alpha Transparent.

This case of bug is able to be fixed it easily.

There is 2 greys as you can see too, the second one is named "Selected" in the named color list but it gets assigned Grey a second time.

In this case, it will not be easy to fix due to the impact of the original implementation. I would try to fix but it would be breaking change.

@custa1200
Copy link

It's "almost" perfect. I also made a formatter for the Hex + Alpha too, I might do a pull request for that.

@custa1200
Copy link

@griffin-stewie would the second one be worth doing and looking to add it to a 2.0 release?

@griffin-stewie
Copy link
Contributor Author

@custa1200 Unfortunately, I'm just a contributor so I can't judge the release.
I think I need to take a closer look at the sphere of influence where I feel it's a problem. I am willing to make changes to this Pull Request if the problem is less severe than I was concerned about.

@custa1200
Copy link

The owner hasn’t picked up your pull requests for a while :( should I spin this out to an issue to track outside this PR?

@griffin-stewie
Copy link
Contributor Author

@custa1200
I pushed some commits to fix problem you mentioned. Could you try this?

@custa1200
Copy link

I tried it out and it worked a treat @griffin-stewie I would say that handles all the color naming issues for me.

@griffin-stewie
Copy link
Contributor Author

@custa1200 I’m glad to hear it! Thank you for testing it.

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