Skip to content

Ms remove dist dir#1164

Merged
rogeruiz merged 8 commits into
18f-pages-stagingfrom
ms-remove_dist_dir
May 17, 2016
Merged

Ms remove dist dir#1164
rogeruiz merged 8 commits into
18f-pages-stagingfrom
ms-remove_dist_dir

Conversation

@msecret

@msecret msecret commented May 11, 2016 •
edited by juliasolorzano
Loading

Copy link
Copy Markdown
Contributor

First draft of attempt to remove dist directory

  • New folder docs/doc_assets holds all js, css, img and font assets that are specific to the docs site and not WDS.
  • Attempts to ensure the dist directory is build before doing anything with docs/website tasks.
  • Copies the js, fonts, and imgs from distto docs/assets/
  • Copies the js, css, fonts, and imgs from docs/doc_assets/ to docs/assets
  • References src/stylesheets for scss paths, and homepage.scss and styleguide.scss @import all.scss from this dir rather then a copied over file.
  • Runs the jekyll build, by now everything should be stored in docs/assets.
  • Ignores docs/assets and dist.

@msecret msecret mentioned this pull request May 11, 2016
Closed
@juliasolorzano juliasolorzano self-assigned this May 12, 2016
juliasolorzano
juliasolorzano reviewed May 12, 2016
View reviewed changes
Comment thread .gitignore Outdated
#
!dist/
dist/
dist-gem/

@juliasolorzano juliasolorzano May 12, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove dist-gem/, as it has been removed and is no longer maintained. See 18F/us_web_design_standards_gem#15 (comment)

@juliasolorzano

juliasolorzano commented May 12, 2016 •
edited
Loading

Copy link
Copy Markdown

Nice work and good start! A few notes from my initial pass:

  • The following files need updates on where users can now download the compiled version of the standards:
    • .github/CONTRIBUTING.md
    • /docs/pages/getting-started.html
  • If I am understanding this correctly, we are using the dist/ directory as a .tmp dir. If so, let's maybe move towards that pathway of the tmp holding tank to help build out what we need. It looks like we have a task started for that in config/gulp/release.js.
  • There was an issue with image paths when building the site locally. I tested out updating the make-images-dir locally and I think this may resolve the issue.

//cc @rogeruiz would be great to get your 👀 on this too!

@msecret

msecret commented May 12, 2016

Copy link
Copy Markdown
Contributor Author

@juliaelman I'm not actually sure what would need to happen in your second point, dist as a tmp dir. Also: it's a temp dir in that it doesn't get checked in, but if you're developing locally or download the zip or from npm, it should exist.

@rogeruiz

rogeruiz commented May 16, 2016

Copy link
Copy Markdown
Contributor

@juliaelman @msecret I can see the confusion around the dist/ directory name for the directory now that we're not checking it in. We could rename it as a tmp/ directory. We would also need to update a few things about the build.

  • Update file paths for the various copy / build / release commands
  • Since the directory is ephemeral, it should be deleted when the build finishes. 🐣
  • Update various documentation to point to the new tmp/ directory

🐣 : This would keep it inline with being a temporary directory. This would also affect where we store the uswds-*.zip file after we build it. This may not be needed if we decide it's easier to keep the directory around.

I'm not sure if going the route of updating the name from dist/ to tmp/ has any major implications and it mostly comes down to semantics, but it does require us to change a few things about how the package is compiled / bundled, the website is built, and the package is released. The dist/ / tmp/ directory is a copy of the distribution files for the package and is used as a temporary directory when building some of the files for the documentation website. Some files are placed in dist/ compiled and ready to be archived into a zip directory ( I think it's specifically only the CSS styles that are not copied out of the dist/ directory since the documentation site works with the Sass style files ). Other files in the same directory are later copied using the gulp website:build from the dist/ directory into the doc/ directory.

All of the above taken into account, the gulp release process already creates a temporary directory named after the package and the current version number. This folder is created, archived, and then deleted. I propose something similar to that for a temporary directory since it communicates that these files shouldn't be relied on.

The original intent around the dist/ directory was to make sure that if someone was building the project from source, they would have the files compiled / bundled on their local machine in a place where they can verify it's contents.

Because so much of the pipeline is dependent on the dist/ directory being named so and having dist/ exist locally for users would want to build the project from source, I think it makes more sense to keep the names of the dist/ directory as is and follow up on updating the documentation related to where users can now download the compiled version.

@juliasolorzano

juliasolorzano commented May 16, 2016

Copy link
Copy Markdown

@rogeruiz thanks for your thoughts on the directory structure! I spoke with @msecret about this to get a better understanding of the intended goals are for this pull request. Here are some of the answers:

  1. The zip file creation is still going to be a manual process.
  2. The /dist folder will likely have it's assets copied around.
  3. The files included in the npm package would stay the same.

Also, thanks all for touching base this morning too! Sounds like we are moving forward, adding some documentation via another PR and working on some fixes.

@msecret msecret force-pushed the ms-remove_dist_dir branch from 910fde1 to 8fe2624 Compare May 17, 2016 19:15
Marco Segreto added 6 commits May 17, 2016 12:21
Removed copied src sass
aa3b3cb 
The src sass is not being copied over into the `docs` folder anymore.

This was done by using the `load_paths` option in the jekyll config
which allows to set a path outside of the docs dir. With this, one load
path is set to src/stylesheets. The references in homepage.scss and
styleguide.scss are both changes to "all" so they import directly from
src.

This means the website always consumes the version of the standards in
the src directory.

TODO: This doesn't use the WDS in a way that a user of them would as
it's a bit of a hack around things.
Move styleguide specific JS to own folder
7345624 
This will let us ignore the whole docs/assets folder
Moved docs CSS to own directory
5f8524c 
This allows us to ignore the whole assets directory

Also updates gitignore to reflect that change
Git ignore docs/assets directory
a77618f
Got images and fonts all working with new flow
2085c2b 
This new flow is
- Copy one js, all fonts and images from `dist` to `docs/assets`.
- Copy all from `docs/docs_assets/` to `doc/assets`.
- Run through rest of build, everything should exist now in
  `docs/assets/`.
Remove dist dir
b924ee8
@msecret msecret force-pushed the ms-remove_dist_dir branch from 8fe2624 to b924ee8 Compare May 17, 2016 19:30
@rogeruiz

rogeruiz commented May 17, 2016

Copy link
Copy Markdown
Contributor

💯

@rogeruiz rogeruiz merged commit 7189023 into 18f-pages-staging May 17, 2016
@rogeruiz rogeruiz deleted the ms-remove_dist_dir branch May 17, 2016 19:44
@msecret msecret mentioned this pull request May 17, 2016
Closed
This was referenced May 18, 2016
Merged
Closed
@rogeruiz rogeruiz mentioned this pull request May 27, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@msecret @juliasolorzano @rogeruiz @maya