Skip to content
This repository has been archived by the owner on Jul 11, 2018. It is now read-only.

update to ionic 3.0.1 #95

Closed
wants to merge 2 commits into from
Closed

update to ionic 3.0.1 #95

wants to merge 2 commits into from

Conversation

peterennis
Copy link

Ionic View App Id 831ef985

I do not see where the maps and some other features are turned on
and I could not find the translation test form.

However this should help close the following:

#88
ionic v3 super template fail with ionic-native ionic-team/ionic-framework#11231
fix(ionic-native): update ionic native to 3.1.0 #84
Missing ionic-native #85
How do I test this template ? #87
Super template not compatible with ionic-native 3+ #86

C:\ae\ionic-starter-super>ionic info

Your system information:

 ordova CLI: 6.5.0
Ionic Framework Version: 3.0.1
Ionic CLI Version: 2.2.1
Ionic App Lib Version: 2.2.0
Ionic App Scripts Version: 1.3.0
ios-deploy version: Not installed
ios-sim version: Not installed
OS: Windows 10
Node Version: v6.10.2
Xcode version: Not installed

capture061

Copy link
Contributor

@janpio janpio left a comment

Choose a reason for hiding this comment

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

Went through it quickly and noticed some things.


// The translate loader needs to know where to load i18n files
// in Ionic's static asset pipeline.
export function createTranslateLoader(http: Http) {
return new TranslateStaticLoader(http, './assets/i18n', '.json');
return new TranslateHttpLoader(http, './assets/i18n', '.json');
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look right, change from Static to Http?

Copy link
Author

Choose a reason for hiding this comment

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

I do not know the internal details. Please feel free to correct as appropriate.

From here: https://github.com/ngx-translate/core

Configuration

By default, there is no loader available. You can add translations manually using setTranslation but it is better to use a loader. You can write your own loader, or import an existing one. For example you can use the TranslateHttpLoader that will load translations from files using Http.

Once you've decided which loader to use, you have to setup the TranslateModule to use it.

Here is how you would use the TranslateHttpLoader to load translations from "/assets/i18n/[lang].json" ([lang] is the lang that you're using, for english it could be en):

Copy link
Contributor

Choose a reason for hiding this comment

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

You are correct:

replace the TranslateStaticLoader ()if you use it) with the TranslateHttpLoader

https://github.com/ngx-translate/core/blob/master/MIGRATION_GUIDE.md#migration-guide-from-ng2-translate-5-to-ngx-translate-6

Copy link
Author

Choose a reason for hiding this comment

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

I did a search and there is no reference to TranslateStaticLoader on the ngx-translate page.
There are some references to "static".
This could be a result of the 5.0.0 to 6.0.1 transition with Angular 4.0.0 but I have not looked into it in depth. You could follow up with the author if needed.

AoT

If you want to configure a custom TranslateLoader while using AoT compilation or Ionic 2, you must use an exported function instead of an inline function.

export function createTranslateLoader(http: Http) {
    return new TranslateHttpLoader(http, './assets/i18n/', '.json');
}

Choose a reason for hiding this comment

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

I think that in line 41 you need to put an ending slash, this:
return new TranslateHttpLoader(http, './assets/i18n', '.json');
to this:
return new TranslateHttpLoader(http, './assets/i18n/', '.json');

package.json Outdated
"ionic-plugin-keyboard"
],
"cordovaPlatforms": [],
"description": "aesuper: An Ionic project"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

package.json in starters only contains the things that are different or additional to the app base that it is combined with: https://github.com/driftyco/ionic2-app-base/blob/master/package.json. You should probably get rid of all this and only keep the translate stuff.

Copy link
Author

Choose a reason for hiding this comment

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

OK. Looks like a leftover artifact from my clone process i.e. pre-fork. I will run a compare with the original and fix it.

@@ -41,18 +41,15 @@ export class ItemCreatePage {
}

getPicture() {
if (Camera['installed']()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this check got lost in the new code

Copy link
Author

Choose a reason for hiding this comment

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

OK

GoogleMap.isAvailable().then(() => {
const position = new GoogleMapsLatLng(43.074395, -89.381056);
this.map.setPosition(position);
this.googleMaps.isAvailable().then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo? this.googleMaps => this.googleMap

Copy link
Author

Choose a reason for hiding this comment

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

Likely. I will make the change. However, I did not see maps in the app. There may still be config issues or operator error.

Copy link
Author

Choose a reason for hiding this comment

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

Looks like not a typo, or there is something wrong with the typescript intellisense. I will leave it as is and someone with more knowledge should check.

capture064

@peterennis
Copy link
Author

There now 4 PR's related to this.
#84, #90, #95, #96
@maxlynch, Here is a request by ionic to test this app:
http://blog.ionic.io/help-us-test-the-super-starter/

It seems to me that the community has responded but ionic is asleep at the wheel.
I would appreciate some input on when this issue will be laid to rest.


// The translate loader needs to know where to load i18n files
// in Ionic's static asset pipeline.
export function createTranslateLoader(http: Http) {
return new TranslateStaticLoader(http, './assets/i18n', '.json');
return new TranslateHttpLoader(http, './assets/i18n', '.json');

Choose a reason for hiding this comment

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

I think that in line 41 you need to put an ending slash, this:
return new TranslateHttpLoader(http, './assets/i18n', '.json');
to this:
return new TranslateHttpLoader(http, './assets/i18n/', '.json');

@danielsogl
Copy link
Contributor

Thank you @peterennis for all your suggestions. Becuase your PR it to large, and it is not possible to split it into smaller commits, I have to close your PR but I implemented your suggestions.

Thank you for your help!

@danielsogl danielsogl closed this May 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants