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

Dark Mode: Primary Labels #4175

Closed
wants to merge 5 commits into from
Closed

Dark Mode: Primary Labels #4175

wants to merge 5 commits into from

Conversation

jjatie
Copy link
Collaborator

@jjatie jjatie commented Oct 15, 2019

Added system dark mode support for primary label colors in the frameworkMoved Color to its own Platform file

Issue Link 🔗

#4133
#3742

Goals ⚽

Introduce dark mode for label and gracefully degrade to black for the older operating systems

Implementation Details 🚧

Introduced NSUIColor.label to match modern (UI/App)Kit semantic colors for earlier versions of the respective OSes.

Testing Details 🔍

N/A

@jjatie
Copy link
Collaborator Author

jjatie commented Oct 15, 2019

@liuxuan30 Let me know if there are any issues.

@jjatie
Copy link
Collaborator Author

jjatie commented Oct 15, 2019

#3766
#4171

@jjatie jjatie mentioned this pull request Oct 15, 2019
@jjatie jjatie requested a review from liuxuan30 October 15, 2019 15:37
Source/Charts/Utils/Platform+Color.swift Outdated Show resolved Hide resolved
Source/Charts/Utils/Platform+Color.swift Outdated Show resolved Hide resolved
follow code style
@liuxuan30
Copy link
Member

liuxuan30 commented Oct 16, 2019

@jjatie I searched NSUIColor.black and shows below, I assume you already checked?
image

There is some colors like
@objc open var borderColor = NSUIColor.black

                attrString = NSMutableAttributedString(string: newValue!)
                attrString?.setAttributes([
                    .foregroundColor: NSUIColor.black,
                    .font: NSUIFont.systemFont(ofSize: 12.0),
                    .paragraphStyle: paragraphStyle
                    ], range: NSMakeRange(0, attrString!.length))
    /// the color drawing borders around the bars.
    open var barBorderColor = NSUIColor.black
open class BarChartDataSet: BarLineScatterCandleBubbleChartDataSet, IBarChartDataSet
{
    private func initialize()
    {
        self.highlightColor = NSUIColor.black

do you think we should also change them?

For the border+highlight colors, I think it may be user's duty to do it, so we should leave it there;
but for the attributed string, I think it's ours?

@codecov-io
Copy link

codecov-io commented Oct 16, 2019

Codecov Report

Merging #4175 into master will increase coverage by <.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4175      +/-   ##
==========================================
+ Coverage   41.47%   41.48%   +<.01%     
==========================================
  Files         119      120       +1     
  Lines       14034    14035       +1     
==========================================
+ Hits         5821     5822       +1     
  Misses       8213     8213
Impacted Files Coverage Δ
Source/Charts/Components/ChartLimitLine.swift 0% <ø> (ø) ⬆️
Source/Charts/Components/AxisBase.swift 64.48% <ø> (ø) ⬆️
Source/Charts/Charts/ChartViewBase.swift 30.22% <ø> (ø) ⬆️
Source/Charts/Utils/Platform.swift 13.33% <ø> (ø) ⬆️
Source/Charts/Components/Description.swift 100% <ø> (ø) ⬆️
Source/Charts/Components/Legend.swift 65.09% <ø> (ø) ⬆️
Source/Charts/Utils/Platform+Color.swift 100% <100%> (ø)
...Charts/Data/Implementations/ChartBaseDataSet.swift 25.82% <50%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25c0742...2de3b5a. Read the comment docs.

@jjatie
Copy link
Collaborator Author

jjatie commented Oct 16, 2019

@jjatie I searched NSUIColor.black and shows below, I assume you already checked?
image

There is some colors like
@objc open var borderColor = NSUIColor.black

                attrString = NSMutableAttributedString(string: newValue!)
                attrString?.setAttributes([
                    .foregroundColor: NSUIColor.black,
                    .font: NSUIFont.systemFont(ofSize: 12.0),
                    .paragraphStyle: paragraphStyle
                    ], range: NSMakeRange(0, attrString!.length))
    /// the color drawing borders around the bars.
    open var barBorderColor = NSUIColor.black
open class BarChartDataSet: BarLineScatterCandleBubbleChartDataSet, IBarChartDataSet
{
    private func initialize()
    {
        self.highlightColor = NSUIColor.black

do you think we should also change them?

For the border+highlight colors, I think it may be user's duty to do it, so we should leave it there;
but for the attributed string, I think it's ours?

I think it's unclear whether or not we should change border and highlight colors, and users have a way to change it themselves. If we decided we should, we can do it in a later PR. Perhaps a separate Color.border and Color.highlight semantic color extensions.

I think you might have an older copy checked out, because I only found one text element I missed.

@liuxuan30
Copy link
Member

let's ignore line color and border colors.
seems @available(tvOS, introduced: 9.0, obsoleted: 13.0) is not working as expected, always return .black color on iOS 13 simulators

extension UIColor
{
@available(iOS, introduced: 8.0, obsoleted: 13.0)
@available(tvOS, introduced: 9.0, obsoleted: 13.0)
Copy link
Member

Choose a reason for hiding this comment

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

API not working

@jjatie jjatie closed this Oct 28, 2019
@jjatie jjatie deleted the feature/label-dark-mode branch October 28, 2019 09:07
@liuxuan30
Copy link
Member

reply from Apple

Apple
Nov 7, 2019 at 8:58 AM
Thanks for your report. `obsoleted` is not designed to work the way you want it to work here—it’s supposed to indicate that the symbol may have been removed by iOS 13, but it will still be used if it’s there. We don’t currently have a “polyfill” feature like the one you’re looking for, but a Swift Evolution pitch adding one might be well-received.

Given the language we have today, the original `labelOrBlack` approach discussed in the PR is probably the cleanest solution. If you’re dead-set on using the name “label”, my best idea is to redeclare the property in an Objective-C category and use the Objective-C runtime from a +load method to conditionally install the implementation if it’s not already present.

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.

3 participants