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

Conversion to webpack - part3 - routes isolation, module resolution, refactoring appStorage #188

Merged
merged 16 commits into from
Mar 24, 2019

Conversation

vitorsemeano
Copy link
Contributor

Here goes the part 3 of the webpack conversion. This PR focus on module resolution for webpack, and it also focus in some relevant topics:

  • Routes isolation: moved all controllers to a specific directory, and moved all routes definition from site.js to routes.js
  • Refactor appStorage: merged all appStorage implementations into one single module
  • Polifys removal: removed a bunch of unnecessary polifys, since the targets for this would be using a recent client

I also tested this PR with android, fixing some issues.

src/bower_components/apiclient/appStorage.js Show resolved Hide resolved
src/bower_components/apiclient/appStorage.js Outdated Show resolved Hide resolved
src/components/appRouter.js Show resolved Hide resolved
src/components/skinManager.js Show resolved Hide resolved
src/components/skinManager.js Show resolved Hide resolved
src/scripts/routes.js Outdated Show resolved Hide resolved
src/scripts/routes.js Show resolved Hide resolved
src/scripts/site.js Show resolved Hide resolved
src/scripts/site.js Show resolved Hide resolved
src/scripts/site.js Outdated Show resolved Hide resolved
Copy link
Contributor

@JustAMan JustAMan left a comment

Choose a reason for hiding this comment

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

No new issues, please address my remaining comments and I feel I'm fine with the PR.

@vitorsemeano
Copy link
Contributor Author

I addresses all our comments, please check them again.

Copy link
Contributor

@JustAMan JustAMan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@dkanada dkanada left a comment

Choose a reason for hiding this comment

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

Just want to make sure the skin related code wasn't for custom CSS or branding? I haven't seen any of that in the frontend yet so maybe it was improperly labeled as the skin.

@vitorsemeano
Copy link
Contributor Author

For what i remember, i discussed that with @thornbill, and he said something like the code was there but it never worked or something similar. That's why i proceeded with removing it.

@JustAMan
Copy link
Contributor

like the code was there but it never worked or something similar

I think that we can re-implement skins later if we want to. Less code to maintain for now is good - we need a clean foundation for building up anyway.

@joshuaboniface
Copy link
Member

@vitorsemeano Looks like we have some conflicts, are you able to resolve those?

@vitorsemeano
Copy link
Contributor Author

I have merged the changes from upstream, but i think we should wait for PR #197 to be merged into upstream before this one.

@joshuaboniface
Copy link
Member

Darn @vitorsemeano #197 caused two more conflicts, able to double-check them?

@vitorsemeano
Copy link
Contributor Author

Can you check if the merge is ok? I didn't tested. I can only test later tonight.

@vitorsemeano
Copy link
Contributor Author

After some mess in this branch trying to rebase it, i think i successfully managed to rebase it as intented.

@vitorsemeano vitorsemeano reopened this Mar 22, 2019
Copy link
Member

@dkanada dkanada left a comment

Choose a reason for hiding this comment

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

You probably don't need to move apiclient since we should pull it out soon, but everything else looks good.

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.

5 participants