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

Make legendRenderer property public in order to be externally customizable #3445

Merged
merged 1 commit into from
Jun 10, 2018

Conversation

nagykatalin
Copy link
Contributor

Goals ⚽

The main idea behind this pull request is to make the legend renderer externally customizable and overridable.

Our customer's request was to detect when the user's clicks on a legend and to highlight corresponding line on the chart.

Implementation Details 🚧

By making public the legendRenderer property (similarly to the renderer property) we would like to use our own legend renderer class which stores the exact location of the labels and in this way we can detect if the user clicked on a label or not.

@codecov-io
Copy link

codecov-io commented May 7, 2018

Codecov Report

Merging #3445 into 4.0.0 will decrease coverage by 0.22%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##            4.0.0   #3445      +/-   ##
=========================================
- Coverage   29.42%   29.2%   -0.23%     
=========================================
  Files         114     117       +3     
  Lines       12605   13301     +696     
=========================================
+ Hits         3709    3884     +175     
- Misses       8896    9417     +521
Impacted Files Coverage Δ
Source/Charts/Charts/BarLineChartViewBase.swift 26.53% <100%> (-0.17%) ⬇️
Source/Charts/Charts/ChartViewBase.swift 24.77% <100%> (+1.69%) ⬆️
Source/Charts/Utils/ViewPortHandler.swift 26.84% <0%> (-7.26%) ⬇️
...ata/Implementations/Standard/PieChartDataSet.swift 35.71% <0%> (-5.96%) ⬇️
...plementations/Standard/LineRadarChartDataSet.swift 16.66% <0%> (-4.39%) ⬇️
...urce/Charts/Formatters/DefaultValueFormatter.swift 57.5% <0%> (-4.04%) ⬇️
Source/Charts/Renderers/YAxisRenderer.swift 56.85% <0%> (-2.65%) ⬇️
...ta/Implementations/Standard/LineChartDataSet.swift 30.76% <0%> (-1.89%) ⬇️
...a/Implementations/Standard/CombinedChartData.swift 58.56% <0%> (-1.81%) ⬇️
Source/Charts/Renderers/BarChartRenderer.swift 45% <0%> (-1.51%) ⬇️
... and 49 more

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 08bd20c...4d2e335. Read the comment docs.

@jjatie
Copy link
Collaborator

jjatie commented May 7, 2018

@nagykatalin I'm okay with this, however please target 4.0.0 instead of master.
@liuxuan30 Any comments?

@nagykatalin nagykatalin changed the base branch from master to 4.0.0 May 7, 2018 16:33
@nagykatalin nagykatalin changed the base branch from 4.0.0 to master May 7, 2018 16:34
@nagykatalin nagykatalin force-pushed the customizable-legend branch from 0b26147 to 4d2e335 Compare May 7, 2018 18:10
@nagykatalin nagykatalin changed the base branch from master to 4.0.0 May 7, 2018 18:10
@liuxuan30
Copy link
Member

liuxuan30 commented May 15, 2018

I'm ok too, giving the fact other major renderers are already public.

But I recommend be careful about the parameters, as LegendRenderer(viewPortHandler: _viewPortHandler, legend: _legend), viewPortHandler may not be available outside.

Oh I just saw 4.0 has made viewPortHandler readable, should be fine.

The remaining question is shall we change

    /// The legend object containing all data associated with the legend
    @objc open internal(set) lazy var legend = Legend()

to full open access for legend object? @jjatie

@liuxuan30 liuxuan30 requested review from liuxuan30 and jjatie May 15, 2018 00:27
@pmairoldi
Copy link
Collaborator

I think legend still being readonly is fine.

@pmairoldi pmairoldi merged commit 52c06d6 into ChartsOrg:4.0.0 Jun 10, 2018
@pmairoldi
Copy link
Collaborator

Thanks 👍

@liuxuan30
Copy link
Member

good to have you back:)

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.

5 participants