Skip to content
This repository was archived by the owner on Nov 2, 2023. It is now read-only.

Utilize Bundler to manage dependencies #86

Merged
merged 8 commits into from
May 8, 2017

Conversation

adamvoss
Copy link
Contributor

NOTE: this is built on #85 so the diff from that will also show here until it is merged. What this does:

  • Adds a relevant Gemfile to that so the command from the Jekyll homepage, bundle exec jekyll serve, works to host the site. This also eliminates the need to manually install dependencies (beyond Ruby/Bundler) locally in order to build.
  • "Un-forks" minima jekyll theme and then utilize theme gem where possible. Results in fewer files to maintain. (I did not spend as much time as I could have working around the access issues I was facing, but some of these deviations were causing serving with bundler to fail).
  • Removes Prism.js dependency, Jekyll can handle syntax highlighting of code at compile time, eliminating the need for client-side JavaScript code. This was the part that depends on the pages being converted to Markdown.
    • Color scheme is preserved. There is a slight difference to the padding around the code, but not enough that I considered it significant or problematic. It also has a slightly different fonts resolution list which results in it rendering code sections in a different font on my system than it had previously, but again I did not find it problematic or significant.

This was done in reaction to some recent comments concerning the process of building the site locally. However, I ended up making this a non-minimal set of changes, including some other things I noticed in the process, to (hopefully) reduce overall review time. These commits can be isolated/separated, if that is preferred.

@Relequestual Relequestual self-requested a review March 24, 2017 09:09
This way they work from both GitHub and the website.
@adamvoss adamvoss force-pushed the bundler branch 2 times, most recently from 7b9e693 to f46e665 Compare March 24, 2017 20:25
@handrews
Copy link
Contributor

handrews commented May 7, 2017

@vossad01 I'm finally trying to look at this and get jekyll running locally so I can look at this and #85. After installing ruby 2.4 and bundler, and checking out this branch from your fork, I'm getting the following errors:

detective-eye:json-schema-org.github.io handrews$ git log --decorate HEAD^..HEAD
* commit f46e6655b761dba0742f8a1bc95aba7bc8a83276 (HEAD -> bundler, vossad/bundler)
  Author: Adam Voss <[email protected]>
  Date:   Thu Mar 23 14:57:07 2017 -0500
  
      Manage dependencies with bundler
detective-eye:json-schema-org.github.io handrews$ bundle exec jekyll serve
Configuration file: /Users/handrews/src/json-schema-org.github.io/_config.yml
Configuration file: /Users/handrews/src/json-schema-org.github.io/_config.yml
            Source: /Users/handrews/src/json-schema-org.github.io
       Destination: /Users/handrews/src/json-schema-org.github.io/_site
 Incremental build: disabled. Enable with --incremental
      Generating... 
  Conversion error: Jekyll::Converters::Scss encountered an error while converting 'assets/main.scss':
                    Undefined mixin 'relative-font-size'. on line 123
jekyll 3.4.3 | Error:  Undefined mixin 'relative-font-size'. on line 123
detective-eye:json-schema-org.github.io handrews$ 

I assume you don't see this error- any ideas?

(also, if you could rebase these PRs onto current master that would be awesome)

I'm terribly sorry for the delay, both Relequestual and I have had different things distracting us recently.

@adamvoss
Copy link
Contributor Author

adamvoss commented May 7, 2017

I assume you don't see this error- any ideas?

This was caused by an compatibility with the latest Minima gem which was released after this was submitted. I restricted it to the version this was developed against and it should work now. While I have a fix for working against the latest Gem, it should be tested which I don't have time for right now.

(also, if you could rebase these PRs onto current master that would be awesome)

At this request, I have brought #85 fully up-to-date with master and rebased this on top of #85. Beyond that I believe I have managed history as best as can be in this situation. Rebasing #85 would either result in either erroneous commits or loss of history and #87 would more easily be recreated than rebased.

Thanks!

@handrews handrews merged commit 1faecdb into json-schema-org:master May 8, 2017
@handrews
Copy link
Contributor

handrews commented May 8, 2017

Thanks for reworking this so quickly, and my apologies again for the delay!

@handrews
Copy link
Contributor

handrews commented May 8, 2017

@vossad01 apparently this is not building, although it built fine for me :-/

The page build failed for the `master` branch with the following error:

Your SCSS file `assets/main.scss` has an error on line 123: Undefined mixin 'relative-font-size'. For more information, see https://help.github.com/articles/page-build-failed-invalid-sass-or-scss/.

For information on troubleshooting Jekyll see:

  https://help.github.com/articles/troubleshooting-jekyll-builds

If you have any questions you can contact us by replying to this email.

My CSS knowledge is so out-of-date that it pre-dates SCSS, and that file is only 48 lines long so I'm a bit puzzled here. I can try to debug it tomorrow but if you have any pointers please let me know.

@adamvoss
Copy link
Contributor Author

adamvoss commented May 8, 2017

@handrews the @import statements expand which is why the line number does not point to a line in that file.

I really cannot explain why you would receive this error. The obvious explanation would be that GitHub pages does not respect the Gemfile and does not provide old version of the Gems; however, that is contradicted by the fact it built successfully in my repo -- only a warning about the CNAME already used elsewhere.

Rather than figuring out what oddity caused it to still build against the old gem, you could accept #99 to just update to the latest.

@adamvoss adamvoss deleted the bundler branch May 10, 2017 00:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants