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

feat(swReference): Reference SW in application #24

Merged
merged 13 commits into from
Jul 27, 2018

Conversation

rishabh3112
Copy link
Contributor

Refers #5
@dhruvdutt @sendilkumarn needs guide to proceed further

@rishabh3112
Copy link
Contributor Author

Have a look @dhruvdutt @sendilkumarn ,
This is intended to fix the issue #5

@rishabh3112 rishabh3112 changed the title feat(swReference): Add html-webpack-plugin feat(swReference): Reference SW in application Jul 24, 2018
@sendilkumarn
Copy link
Owner

@rishabh3112 awesome 🎉
Can you let me know what guidance you need on this?

@rishabh3112
Copy link
Contributor Author

rishabh3112 commented Jul 24, 2018

@sendilkumarn I found some bugs while testing like:

  1. Output folder is set to be /dist instead of ./dist.
  2. Manifest is moved in dist folder under dist folder
    Can I correct them in this PR itself?

If you have a look at my 2nd commit can you suggest that the approch with I am proceeding is correct in terms of maintainability or not.

@dhruvdutt
Copy link
Contributor

@rishabh3112 Nice job. 💯

You can go ahead and fix the bugs you've identified. The fix for these looks like something which is out of scope of this PR. You can add a new PR preferably and then rebase this one.

@rishabh3112
Copy link
Contributor Author

@dhruvdutt can you check this up

@rishabh3112
Copy link
Contributor Author

Need not merge #22 Now

@rishabh3112
Copy link
Contributor Author

Any feedback on this PR @sendilkumarn @dhruvdutt ??

Copy link
Contributor

@dhruvdutt dhruvdutt left a comment

Choose a reason for hiding this comment

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

The changes look good. 💯 Haven't checked it locally.

Sorry for the late review.

Copy link
Owner

@sendilkumarn sendilkumarn left a comment

Choose a reason for hiding this comment

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

Awesome merging this. 👍

@sendilkumarn sendilkumarn merged commit 7c0a126 into sendilkumarn:master Jul 27, 2018
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.

None yet

3 participants