Skip to content

Conversation

@shawnbot
Copy link
Contributor

I made some updates to the README for the monorepo. If it all looks good, this should be merged into mono before merging #230. Here's what's changed:

  1. For consistency, "npm" is always written in lowercase.
  2. Add the sh language identifier to each of the shell examples.
  3. Replace "(scss)" with "(SCSS)" after the Sass link.
  4. Replace the guidance on referencing submodules via /lib/ with instructions for installing individual modules (e.g. primer-navigation) and an example of linking to submodules in the packages directory of this repo for users who are still figuring out which modules they need.

@shawnbot shawnbot requested review from broccolini and jonrohan June 22, 2017 19:37
@shawnbot shawnbot changed the base branch from mono to master June 23, 2017 23:17
Copy link

@sophshep sophshep left a comment

Choose a reason for hiding this comment

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

Left one pedantic comment but otherwise looks great!

README.md Outdated
## Usage

The source files included are written in [Sass][sass] (`scss`) You can simply point your sass `include-path` at your `node_modules` directory and import it like this.
The source files included are written in [Sass][sass] (SCSS). You can point your sass `include-path` at your project's `node_modules` directory and import it like this:

Choose a reason for hiding this comment

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

I hate myself for this comment but we should capitalize Sass in You can point your sass

Copy link
Contributor

@broccolini broccolini left a comment

Choose a reason for hiding this comment

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

👍

@broccolini broccolini merged commit 4c5b56e into master Jun 28, 2017
## Usage

The source files included are written in [Sass][sass] (`scss`) You can simply point your sass `include-path` at your `node_modules` directory and import it like this.
The source files included are written in [Sass][sass] (SCSS). You can point your Sass `include-path` at your project's `node_modules` directory and import it like this:
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll have this provided already with the website-starter repo, but others might not know what include-path is. Can we give context on this for Jekyll, or link to an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Can we link to a URL about Sass include paths more generally?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @mdo. @shawnbot Would you be down to open a new pr? This doesn't have to hold up the new release today, but should be added.

```

You can also import specific portions of the module by importing those partials from the `/lib/` folder. _Make sure you import any requirements along with the modules._
You can import individual Primer modules by installing them each with npm, for instance:
Copy link
Contributor

Choose a reason for hiding this comment

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

installing them with instead of installing them each with?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can fix that one the new release pr.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed here: 59fea15

@broccolini broccolini deleted the mono-patch-readme branch June 28, 2017 22:12
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.

5 participants