cds-snc / e-briefing-app

Ionic-based cross platform mobile app for the e-briefing service
MIT License
2 stars 4 forks source link

Refactoring #6

Closed CalvinRodo closed 5 years ago

CalvinRodo commented 6 years ago

I did some refactoring on the app that should make it a bit easier to maintain going forward.

Added aliases for paths so that imports for modules from ./src/app, ./src/providers and ./src/pages no longer have to be relative to the file they are being imported to they can now be imported using @app, @providers, and @pages respectively.

Added ApiProvider to reduce the copy pasted code in the various providers.

Moved providers to a shared module to clean up the app.modules.ts file.

*I haven't tested the api changes since I don't have an instance of the server running so can't guarantee that there are no bugs.

CalvinRodo commented 6 years ago

I'd fix the dependency issues but I can't view them.

dsamojlenko commented 6 years ago

Calvin, this is great. I will pull down and test as soon as I can. I can hit you by email with access to the api backend if you'd like.

CalvinRodo commented 6 years ago

That would be handy, I was also going to add some localizations to the app as well.

ESDC is getting in the app game figured helping another team was better then me playing around on a hello world.

CalvinRodo commented 6 years ago

Now that I know the pages are reading from a file the ApiProvider is probably poorly named.

Also still getting used to pull issues on github, haven't done a tonne didn't mean to push the translation demo as well, I'll bring that back and fork from my master.

CalvinRodo commented 6 years ago

Should be fixed now.

There was a breaking change when the dependencies were updated with how webpack was overridden.

dsamojlenko commented 5 years ago

Hey, finally got around to merging this - sorry for the delay ... had to make a few minor changes - the template literals you were using for the urls being passed to fileReader were not being evaluated - had to use backticks:

// this:
this.fileReader.getJson('data/documents/${id}.json', this.data);

// should be:
this.fileReader.getJson(`data/documents/${id}.json`, this.data);

and a few of the paths were missing a /

otherwise, thanks a lot for this - definitely makes the code more readable/maintainable!