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

#470 Change default find highlight color #481

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mohanio
Copy link

@mohanio mohanio commented Oct 19, 2019

Summary

Issue:
#470
Find highlight color defaults to red color instead it should use system's default find highlight color(NSColor.findHighlightColor)

Fix:

  • Theme has new static variable representing default find highlight color
  • Other find highlight colors are generated from the hue of default find highlight color
  • Find highlight color specified by xi-editor on theme_changed event is ignored

Related Issues

closes #470

Checklist

Example:

  • Example check-list item
  • Create a pull request template
  • make xi perfect

Review Checklist

  • I have responded to reviews and made changes where appropriate.
  • I have tested the code
  • I have updated comments / documentation related to the changes I made.
  • I have rebased my PR branch onto xi-mac/master.

Copy link
Member

@nangtrongvuon nangtrongvuon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Overall looks good. but you shouldn't have the issue link as the commit message. Instead, describe what you did, and then put the link in the commit message body instead.

@@ -82,7 +93,8 @@ extension Theme {
let caret = NSColor(jsonRgbaColor: json["caret"] as? [String: Any] ?? [:])
let line_highlight = NSColor(jsonRgbaColor: json["line_highlight"] as? [String: Any] ?? [:])

let find_highlight: NSColor? = NSColor(jsonRgbaColor: json["find_highlight"] as? [String: Any] ?? [:])
// Use default find highlight color instead of theme's find_highlight value
let find_highlight: NSColor? = Theme.defaultFindHighlightColor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if the theme specifies a color for find highlights, then it's ok to use that color instead of forcing the default yellow.

Copy link
Author

@mohanio mohanio Oct 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'm also bit not clear on expected behavior. I will wait for @cmyr's clarification #470 (comment)

Issue:
xi-editor#470
Find highlight color defaults to red color instead it should use system's default find highlight color(NSColor.findHighlightColor)

Fix:
* Theme has new static variable representing default find highlight color
* Other find highlight colors are generated from the hue of default find highlight color
* Find highlight color specified by xi-editor on theme_changed event is ignored
@mohanio mohanio force-pushed the 470-set-default-find-highlight-color-to-system-default branch from 3ad5d49 to df6d765 Compare October 21, 2019 17:19
@mohanio
Copy link
Author

mohanio commented Oct 21, 2019

Thanks for this! Overall looks good. but you shouldn't have the issue link as the commit message. Instead, describe what you did, and then put the link in the commit message body instead.

Updated the commit subject and message.

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.

Change default find highlight color
2 participants