Skip to content

Conversation

@JPrevost
Copy link
Member

I couldn't make the main example work without renaming it to example.md... but I left it as HTML so 🤷‍♀️

I had to add jshint to package.json to get it to run the gulp task... but I also have no idea what I'm doing so please test this locally!!!!!!

The Gemfile is not entirely necessary, but it makes developing locally way easier. If you prefer this is not in the repo let me know and I can remove it.

I moved a lot of stuff around to make life with jekyll easier. It seems fine this way, but thoughts are welcome.

I only left the core examples in example.md the layout has been moved to _layouts which calls a series of partials from _includes.

I left templates/ebooks-complete.html alone. It can be moved to the new layouts without much effort but I'll leave that as a lesson to the reader ;)

@frrrances
Copy link
Contributor

Thanks, @JPrevost ! This is most excellent. jshint is apparently required with gulp-jshint above version 2 so that's all :shipit: . Gemfile is making local a lot nicer so 💯 to that. Moves, underscores, and renames are all 🌈 . I'll convert ebooks in a new PR. And I'll get @thatandromeda to review just to check our bizness. ✨

@frrrances frrrances requested a review from thatandromeda July 13, 2017 14:22
Copy link
Contributor

@thatandromeda thatandromeda left a comment

Choose a reason for hiding this comment

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

Ahhhhh this is super great - both having it Jekyll-ified for ease of messing around on local, and also having all these modular chunks laid out everywhere and the super thorough example page.

Question - does this supersede http://libraries.mit.edu/style-guide/ ? Should that page just a pointer to the repo now? Or are things different enough on WordPress that that page merits independent existence?

Two things I would like to see addressed before merge:

  • [ ] The broken link I commented about elsewhere
  • [ ] Instructions in the README for running Jekyll to see changes locally

<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Open+Sans:400,300,300italic,400italic,600,600italic,700,700italic&subset=latin,latin-ext" type="text/css">
<link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/font-awesome/4.6.1/css/font-awesome.min.css">
<link rel="stylesheet" href="{{ '/css/styles.css?v=' | append: site.github.build_revision | relative_url }}">
<link rel="stylesheet" href="/dest/css/libraries-main.css">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we using this link rather than the template-language version? (Also, my local Jekyll complains that it is not found.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is a result of using these partials as both something you can grab and use in a project and to run this style guide. That libraries-main.css needs to be included in the head of any project that wants to use the style guide, but is compiled in a different place for this standalone guide. I figured the most useful thing would be to put the link in the head partial so it's easy to grab even though the link will be broken when running the guide on it's own. Let me know what you think about it with that context...

@frrrances
Copy link
Contributor

frrrances commented Jul 13, 2017

Thanks for the review @thatandromeda ! I commented on the broken link; as for your question about the guide on the website... I'm working toward replacing that, but on the website that older style guide is still what's applicable to the site as it doesn't use this new style guide code to run the site. So for now, they need to both exist.

I may need @JPrevost to review the Jekyll instructions, so I'll make a new PR for that. Sound good?

thatandromeda
thatandromeda previously approved these changes Jul 13, 2017
@thatandromeda
Copy link
Contributor

OK, separate PR for Jekyll instructions is 🌈 . Can you add the broken-link explanation as a comment in the html file so it is obvious in context to future users? Then go ahead and merge.

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.

4 participants