-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
Added support for user colors #132
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with minor comment
src/marks/mark.py
Outdated
"-userColor1": "userColor1", | ||
"-userColor2": "userColor2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we give these real spoken forms? Eg "navy" and whatever the closest match is for the default you've used for color 2? Also, I'd be inclined not to include the -
, because they will only appear in the csv if the user has actually turned them on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure. I have really not found a good second color so whatever we name the second color is probably not going to be what the user actually ends up using.
We can remove the minus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just a bit hesitant to use camel case in the spoken form column. Why not just call them "navy" and "salmon"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do that if you prefer it. I can see if I can find a better second color and then update the names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second color looks close enough to salmon to me. I would be inclined to just change the spoken form to "salmon" and ship it. But if you wanna keep tweaking the colors go for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The color right now is apricot. I will take another crack at it before deciding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find a better color that didn't conflict with the others visually. But the colors are now renamed to navy and apricot.
2c1634e
to
de46f67
Compare
Added nine configurable colors for the user. I'm going to experiment with more colors so I had need of this myself.