Skip to content
This repository has been archived by the owner on Dec 18, 2019. It is now read-only.

WIP: AEROGEAR-8989 - Showcase app config #674

Merged
merged 7 commits into from
Apr 11, 2019
Merged

WIP: AEROGEAR-8989 - Showcase app config #674

merged 7 commits into from
Apr 11, 2019

Conversation

jstaffor
Copy link
Contributor

@jstaffor jstaffor commented Apr 4, 2019

@jstaffor
Copy link
Contributor Author

jstaffor commented Apr 4, 2019

@austincunningham - ready for a review 👍


. Open your {mobile-client} in Mobile Developer Console.
. Copy the `mobile-services.json` configuration to your clipboard.
. Save the contents of the clipboard to a new file called `mobile-services.json`.
Copy link
Member

Choose a reason for hiding this comment

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

This is for the showcase app, which will have a mobile-service.js file already in git https://github.com/aerogear/ionic-showcase/blob/master/src/mobile-services.js , don't think there is any need to create mobile-service.json. Replace the contents of the config object in mobile-services.js with the contents of mobile-service.json. i.e. everything between the {} inclusive . This is really hard to describe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't follow @austincunningham - would you mind making this technical change and I'll review it from a docs point of view (if there is a need)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think I know what you are asking. Let me give it a go.

Copy link
Member

Choose a reason for hiding this comment

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

As the show case is only an ionic app, there is no need for the configs for the other platforms,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that there is no need for the configs for the other platforms, I've removed them. In addition to this, I've added a note to this section to make it clear to the reader that this section is only for the Ionic platform.

That addresses your feedback. Can you review it again please?

@jstaffor
Copy link
Contributor Author

jstaffor commented Apr 4, 2019

@austincunningham - ready for review.

Copy link
Member

@austincunningham austincunningham 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 remove that line as no need to make a local copy of mobile-service.json as it is on the clipboard .

@jstaffor
Copy link
Contributor Author

jstaffor commented Apr 5, 2019

@finp - this is ready for a final review

@jstaffor
Copy link
Contributor Author

jstaffor commented Apr 5, 2019

Just a note: I think this work is related to #551

Copy link
Member

@austincunningham austincunningham left a comment

Choose a reason for hiding this comment

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

LGTM

@pwright
Copy link
Member

pwright commented Apr 8, 2019

@austincunningham does this make sense for ionic?
image

or is it just that it doesn't make sense for showcase?

@jstaffor jstaffor changed the title AEROGEAR-8989 - Showcase app config WIP: AEROGEAR-8989 - Showcase app config Apr 8, 2019
@jstaffor
Copy link
Contributor Author

jstaffor commented Apr 8, 2019

After discussing on docs stand-up, this work is now parked.

@pwright
Copy link
Member

pwright commented Apr 9, 2019

Update: the documentation issue for ionic will be resolved by aerogear/aerogear-js-sdk#318
I'm not sure when this will land, but ask "does it make sense to avoid updating ionic docs until then?"

@pwright pwright merged commit a5768ac into aerogear:master Apr 11, 2019
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.

3 participants