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

Restructure project src & fixed some other issues #27

Merged
merged 3 commits into from
Oct 22, 2017
Merged

Restructure project src & fixed some other issues #27

merged 3 commits into from
Oct 22, 2017

Conversation

unixzii
Copy link
Contributor

@unixzii unixzii commented Oct 16, 2017

Fixes #25.

Changes include:

  • Restructured project source folders
  • Optimized .gitignore file

@jh3y

public/

test/test.js
dist/
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure if dist should be ignored. If someone pulls this package in via different means, this forces them to create the output files themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, we should always avoid committing dist or build stuff. For GitHub users, we can publish distributions to the release page so that they can directly download the distribution without clone the whole repo.

Copy link
Owner

Choose a reason for hiding this comment

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

Had completely forgot about attaching files to the release 😄 great shout!

Makefile Outdated
@@ -13,8 +13,8 @@ GHPAGES = $(MODULES)/gh-pages

DEST = dist
FILE_NAME = ep
SCRIPT_SRC = src/script/entries/ep/index.js
STYLE_SRC = src/script/entries/ep/ep.scss
SCRIPT_SRC = src/script/entries/ep/scripts/index.js
Copy link
Owner

Choose a reason for hiding this comment

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

I should have maybe been more clear in my wording within the issue. My aim was to make the structure more intuitive in the sense of extracting styles from the src/script folder.

Something like;

- src
  - script
    - index.js
  - styles
    - ep.scss

@@ -4,6 +4,7 @@
"description": "enhance your progress bars with minimal effort!",
"main": "dist/ep.js",
"scripts": {
"prepublish": "make dist",
Copy link
Owner

Choose a reason for hiding this comment

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

If I understand correctly, this is why dist has been ignored? It's assumed that a user will pull the package in via npm and therefore creating the dist output prepublish makes it available to those who pull in via npm etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, this script make npm build the package before it is published to the npm central repository, therefore after npm users installing the package, the dist will just be there. In this way, we can guarantee that the build is always newest and relax us from manually building.

Copy link
Owner

@jh3y jh3y left a comment

Choose a reason for hiding this comment

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

Thanks for contributing @unixzii 👍

I've just got a couple of comments regarding the changes 😄

Copy link
Owner

@jh3y jh3y left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

Just one last thing, can we bump the version number before squash and merge? 😄

@unixzii
Copy link
Contributor Author

unixzii commented Oct 22, 2017

@jh3y Okay, done.

@jh3y jh3y merged commit b74ce88 into jh3y:master Oct 22, 2017
@jh3y jh3y mentioned this pull request Dec 18, 2017
@luncht1me
Copy link

luncht1me commented Oct 18, 2018

imo, semver should've changed to 2.1 with this change -- mainly because bower installs are broken if were relying on the dist/ files as bower doesn't build them out with current config. I have a production site I had to lock in to 2.0.3 as ^2.0.3 on a fresh install updated it which lost the dist files in which we relied on.

I'll figure it out in some free time and patch it so bower install is no longer without any dist files.

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.

3 participants