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

Support other bundle than main MarkerView.viewFromXib() #3215

Merged
merged 4 commits into from
Jan 30, 2018
Merged

Support other bundle than main MarkerView.viewFromXib() #3215

merged 4 commits into from
Jan 30, 2018

Conversation

charlymr
Copy link
Contributor

Hi there;

why this pull request:

If you use the Charts Framework from another Bundle, this method will not find the ressources as this is looking in the main bundle only.

I added a method to be able to load the ressources from a given bundle.

Thanks,

Denis

Copy link
Collaborator

@jjatie jjatie left a comment

Choose a reason for hiding this comment

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

Looks good. Just updated based on review and we'll merge for 3.1

@@ -72,10 +72,15 @@ open class MarkerView: NSUIView, IMarker
}

@objc
open class func viewFromXib() -> MarkerView?
open class func viewFromXib() -> MarkerView? {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please put the { on a new line.

}

@objc
open class func viewFromXib(in bundle: Bundle) -> MarkerView?
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be one method: open class func viewFromXib(in bundle: Bundle = .main) -> MarkerView? instead of two.

Copy link
Contributor Author

@charlymr charlymr Jan 26, 2018

Choose a reason for hiding this comment

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

Yeah that's what I was thinking in the first place, however if someone is overriding that method it will break his code. (@objc) is declared as well here. So even more important!

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The main reason to override this method is to change the bundle. If it breaks their code, this is a very minor change and we will have a few possible minor code breaking changes in 3.1.

Let's make this one method.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And keep the { on a new line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All done. Thanks. Great job by the way :)

@jjatie jjatie added this to the 3.1 milestone Jan 26, 2018
@jjatie jjatie added this to Done in 3.1 release Jan 26, 2018
@jjatie jjatie moved this from Done to In progress in 3.1 release Jan 26, 2018
@jjatie
Copy link
Collaborator

jjatie commented Jan 26, 2018

Please update the macOS demo for the new method signature.

@liuxuan30 liuxuan30 moved this from In progress to To Do in 3.1 release Jan 29, 2018
@liuxuan30 liuxuan30 moved this from To Do to In progress in 3.1 release Jan 29, 2018
@codecov-io
Copy link

Codecov Report

Merging #3215 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3215   +/-   ##
=======================================
  Coverage   22.89%   22.89%           
=======================================
  Files         116      116           
  Lines       15609    15609           
  Branches      272      272           
=======================================
  Hits         3573     3573           
  Misses      12000    12000           
  Partials       36       36
Impacted Files Coverage Δ
Source/Charts/Components/MarkerView.swift 0% <0%> (ø) ⬆️

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 48bc049...4d85f37. Read the comment docs.

@jjatie
Copy link
Collaborator

jjatie commented Jan 30, 2018

@liuxuan30 I approve, I'll give you a chance to review. I know you're busy.

@@ -72,10 +72,10 @@ open class MarkerView: NSUIView, IMarker
}

@objc
open class func viewFromXib() -> MarkerView?
open class func viewFromXib(in bundle: Bundle = .main) -> MarkerView?
Copy link
Member

Choose a reason for hiding this comment

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

I don't realize @objc can be used for functions with default parameters.. :)

@liuxuan30
Copy link
Member

Seems fine. Though busy, I can afford some time every day in the morning.

@liuxuan30 liuxuan30 merged commit cbaf51c into ChartsOrg:master Jan 30, 2018
3.1 release automation moved this from In progress to Done Jan 30, 2018
FreddyZeng added a commit to FreddyZeng/Charts that referenced this pull request Jan 31, 2018
* 'master' of https://github.com/danielgindi/Charts:
  Restored old performance (ChartsOrg#3216)
  Support other bundle than main MarkerView.viewFromXib() (ChartsOrg#3215)
  For ChartsOrg#2840. add dataIndex parameter in `highlightValue()` calls (ChartsOrg#2852)
  Balloon Marker indicates position of data (ChartsOrg#3181)
  bump Info.plist version
  BubbleChart uses correct colour for index now.

# Conflicts:
#	Source/Charts/Mark/BalloonMarker.swift
kzyryanov pushed a commit to fishbrain/Charts that referenced this pull request Feb 20, 2018
* - Added optional  in bundle to the viewFromXib()

* Small styling fix

* Keep only one method with Optional load from a bundle

* Fixed new method signature in Obj-C Demo
kalmurzayev pushed a commit to kalmurzayev/Charts that referenced this pull request Feb 26, 2018
* - Added optional  in bundle to the viewFromXib()

* Small styling fix

* Keep only one method with Optional load from a bundle

* Fixed new method signature in Obj-C Demo
FreddyZeng added a commit to FreddyZeng/Charts that referenced this pull request Mar 14, 2018
* master: (55 commits)
  Refactors -[tableView:cellForRowAtIndexPath:] (ChartsOrg#3326)
  fix ChartsOrg#3311. Need one more key for iOS 11 camera roll saving
  revert a mistake, fix ChartsOrg#3299
  add pie chart unit tests (ChartsOrg#3297)
  ChartsOrg#3287: align Objc and Swift demos balloon marker
  update changelog
  Min and Max reset when clearing ChartDataSet (Fixes ChartsOrg#3260)
  Restored old performance (ChartsOrg#3216)
  Support other bundle than main MarkerView.viewFromXib() (ChartsOrg#3215)
  For ChartsOrg#2840. add dataIndex parameter in `highlightValue()` calls (ChartsOrg#2852)
  Balloon Marker indicates position of data (ChartsOrg#3181)
  bump Info.plist version
  Fixed a duplicated assignment compared with obj-c code. (ChartsOrg#3179)
  Updated README for 3.0.5 (ChartsOrg#3183)
  BubbleChart uses correct colour for index now.
  Added custom text alignment for noData
  Fixes the distance issue between the legend and the horizontal bar chart (Fixes ChartsOrg#2138) (ChartsOrg#2214)
  call setNeedsDisplay() here to trigger render noDataText (ChartsOrg#3198)
  添加定制TY的Mark
  添加修改过的Mark到工程中
  ...

# Conflicts:
#	ICXCharts.podspec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
3.1 release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants