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/build chain #39

Merged
merged 22 commits into from
Apr 21, 2020
Merged

Conversation

FloChehab
Copy link
Contributor

@FloChehab FloChehab commented Apr 19, 2020

Related to #38

Biggest changes

  • Added a development and production setup (webpack, etc.).

Other notable changes

  • Moved some files
  • Updated Dockerfile for lighter image size
  • Reduced "bundle" size by including only some specific icons
  • Removed all hard-coded libs from the repo
  • Changed the color picker lib (I couldn't make the other one work with the new setup)
  • Some very small tweaks due to undefined vars errors, etc.

NB

  • The github diff is a bit off for the js modules that have been moved (I did my best to move the files with git before editing them to keep track of the history).
  • I haven't tested it completely yet.

@cracker0dks
Copy link
Owner

cracker0dks commented Apr 19, 2020

nice, not expected this so fast. I like it and could not find any erros so far 👍 . The colorpicker is maybe even better for small (touch)screens. Thanks

@FloChehab
Copy link
Contributor Author

FloChehab commented Apr 19, 2020

nice, not expected this so fast. I like it and could not find any erros so far +1 . The colorpicker is maybe even better for small (touch)screens. Thanks

Thanks for the feedback !
If you look at the commit, you'll see that I started working on it yesterday haha

Anyway, there are still three small things I am not fully satisfied with

  • What's the best way to pin the Js dependencies,
  • I think we should change a bit the folder setup:
    • Maybe having a static or dist folder with the bundling result,
    • And the public folder for the uploads (currently they will be removed on every build, as the public folder content is cleared every time) -- I'd like to prevent data losses for people upgrading the app.
    • (And play with the express server to have it work as previously.)
  • I am not sure the font-awesome icons work for the dynamically added elements.

@cracker0dks
Copy link
Owner

mmm must admit I have not much exp with webpack so don't know the best way to solve your questions.
However for docker: you have to mount a dir to the public folder anyway. :)

@FloChehab FloChehab marked this pull request as ready for review April 20, 2020 09:05
@FloChehab
Copy link
Contributor Author

mmm must admit I have not much exp with webpack so don't know the best way to solve your questions.
However for docker: you have to mount a dir to the public folder anyway. :)

  • I changed the output of the compiled Js / html / images to /dist.
  • I updated the backend server script so that in continues to serve uploads from /public/uploads (so people can continue mounting volume on /public ; and there won't be data losses).
  • I pinned Js dev-deps with ^
  • Updated the README to reflect the changes

At the end, if people are using docker to deploy the project, there shouldn't be breaking changes with this PR.

* setup is not perfect but should do the trick for now
* Suggestion: add all the icon the dom and reuse them so that we don't have to do dom.i2svg(); everytime
@cracker0dks cracker0dks merged commit ec7bc93 into cracker0dks:master Apr 21, 2020
@cracker0dks
Copy link
Owner

Thanks again. I released version 1.5 and then merged. I think this will be the start of v2.x 🥳

@FloChehab FloChehab deleted the feat/build_chain branch April 21, 2020 12:00
@FloChehab
Copy link
Contributor Author

Thank you @cracker0dks ; I wish I have had time to rebase but that's fine :)

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