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

Feature/webpack #59

Merged
merged 15 commits into from
Oct 26, 2019
Merged

Feature/webpack #59

merged 15 commits into from
Oct 26, 2019

Conversation

chris--jones
Copy link
Contributor

@chris--jones chris--jones commented Oct 20, 2019

This is to address #35 , but is also probably a good step towards modernization as suggested in #44 .

Note: I still intend to do some further optimisation on this, but as it requires a big review I figured I'd submit it early.

Some areas of interest/concern are as follows:

  • I removed the dist folder and git ignored it. Why include the dist with the source? I figured that the sourcecode is for development and having a dist just means extra code to download and a chance it'll be out-of-sync with the source code.
  • I removed the factory logic at the start of the script as it no longer seemed necessary and conflicted with the jQuery import.
  • I replaced the css with less as it was very repetitive and full of vendor prefixes which can be auto-generated (I'll add post css config in a subsequent commit to this branch).
  • The testing index.html page no longer depends on jQuery as a bower install (who uses bower these days?) instead opting for a cdn reference.
  • I removed percircle-rtl.css - assuming this is for right-to-left reading support and that we can generate an rtl stylesheet with a webpack plugin.

TODO:

  • PostCss for vendor prefixes and minification
  • Separate the javascript into separate files for easier maintenance (it's all gonna get bundled now anyway 😄 )

@teobais
Copy link
Owner

teobais commented Oct 20, 2019

@chris--jones thanks a lot for this one, it's super neat!

Just one remark from my side.
It was decided to first merge #57 , so that old users can have this (#57 ) final version of the component itself before we move to a completely new era.

Having said that, could we please fix the conflicts on this PR?
Once that is done, we start moving to webpack and react (I just adjusted the title of #44 ).

Hope that doesn't create much struggle and I need to repeat: your work is great!

@teobais
Copy link
Owner

teobais commented Oct 20, 2019

@chris--jones in addition, here are my comments on your points:

  • dist folder was an old decision made to ease serving the .github.io demo page of the component. It's not a good solution and we can remove it, I concur.
  • cool!
  • cool!
  • indeed, our bower dependency was an old remnant that should have been updated quite earlier, please go ahead :-)
  • great

This is to address #35 , but is also probably a good step towards modernization as suggested in #44 .

Note: I still intend to do some further optimisation on this, but as it requires a big review I figured I'd submit it early.

Some areas of interest/concern are as follows:

  • I removed the dist folder and git ignored it. Why include the dist with the source? I figured that the sourcecode is for development and having a dist just means extra code to download and a chance it'll be out-of-sync with the source code.
  • I removed the factory logic at the start of the script as it no longer seemed necessary and conflicted with the jQuery import.
  • I replaced the css with less as it was very repetitive and full of vendor prefixes which can be auto-generated (I'll add post css config in a subsequent commit to this branch).
  • The testing index.html page no longer depends on jQuery as a bower install (who uses bower these days?) instead opting for a cdn reference.
  • I removed percircle-rtl.css - assuming this is for right-to-left reading support and that we can generate an rtl stylesheet with a webpack plugin.

TODO:

  • PostCss for vendor prefixes and minification
  • Separate the javascript into separate files for easier maintenance (it's all gonna get bundled now anyway 😄 )

add browserslist config to package.json
add environment configs to webpack build scripts
convert css to less
add browserslist config to package.json
add environment configs to webpack build scripts
rename bundle js (back to percircle.js)
add npmignore to minimise npm package size
@chris--jones
Copy link
Contributor Author

Ok, I've added the vendor prefixes, rtl support and minification.
I think I'll handle restructuring in a separate PR.

Copy link
Owner

@teobais teobais left a comment

Choose a reason for hiding this comment

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

LGTM

@teobais teobais merged commit e56b6d8 into teobais:master Oct 26, 2019
@teobais
Copy link
Owner

teobais commented Oct 26, 2019

Thanks a lot for this one @chris--jones , that's a cool makeover of percircle!
Looking forward to a PR for #44 as well :-D

Thanks again 🎉

@teobais
Copy link
Owner

teobais commented Oct 26, 2019

@chris--jones just a small point, but it's a bit flaky on development mode:

Screenshot 2019-10-26 at 13 30 03

I believe the resolution of that fix depends on our plans to migrate to react.js .
That said, if #44 could happen soon, this is automatically "deferred".

@chris--jones
Copy link
Contributor Author

@chris--jones just a small point, but it's a bit flaky on development mode:

Screenshot 2019-10-26 at 13 30 03

I believe the resolution of that fix depends on our plans to migrate to react.js .
That said, if #44 could happen soon, this is automatically "deferred".

Oh sorry, I saw this and immediately knew what it was - I renamed the bundle javascript to test the npm packaging and forgot to check in the updated index.html.
it's literally just the script reference:

  <script src="./percircle.bundle.js"></script>

should be

  <script src="./percircle.js"></script>

PR incoming...

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.

2 participants