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

Added language setting. #15

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sladage
Copy link

@sladage sladage commented Jan 25, 2017

Added the language setting to the load function.

Copy link
Owner

@roberthartung roberthartung left a comment

Choose a reason for hiding this comment

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

See comments

lib/charts.dart Outdated
@@ -48,18 +48,13 @@ abstract class Chart {
jsChart.callMethod('clearChart');
}

static Future load({List<String> packages, String version: "current"}) {
static Future load({List<String> packages, String version: "current", String language}) {
Copy link
Owner

Choose a reason for hiding this comment

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

language should have a default value or be before the version parameter!

Copy link
Author

Choose a reason for hiding this comment

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

No default value must be set since the js lib will try to find the best match for the language option when no language option is provided. Wasn't aware optional parameters which do not have a default values should be placed before those which do. Will change.

lib/charts.dart Outdated
if (packages != null) argsMap["packages"] = packages;
if (language != null) argsMap["language"] = language;

JsObject args = new JsObject.jsify({'callback': new JsFunction.withThis(c.complete)});
Copy link
Owner

Choose a reason for hiding this comment

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

argsMap is not used here?

Copy link
Author

Choose a reason for hiding this comment

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

Good point will check.

@@ -70,18 +70,13 @@ abstract class Chart {
jsChart.callMethod('clearChart');
}

static Future load({List<String> packages, String version: "current"}) {
static Future load({List<String> packages, String version: "current", String language}) {
Copy link
Owner

Choose a reason for hiding this comment

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

same

if (packages != null) argsMap['packages'] = packages;
if (language != null) argsMap['language'] = language;

JsObject args = new JsObject.jsify(argsMap);
Copy link
Owner

Choose a reason for hiding this comment

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

callback missing?

Copy link
Author

Choose a reason for hiding this comment

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

Callback is already added to the argsMap.

@sladage
Copy link
Author

sladage commented Oct 9, 2017

Changes committed.

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.

2 participants