-
Notifications
You must be signed in to change notification settings - Fork 3
Static site generation for GitHub pages #18
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
base: master
Are you sure you want to change the base?
Conversation
|
Aside from the technicalities, and I hate to be that guy, but are you going to have some of this designed at all, @chrishutchinson? It looks odd to me, like with a few things slightly off. |
|
@basilesimon Agree entirely. I will be putting it past Matt M as we did with the data vis catalogue before merging and releasing, but wanted overall agreement from the team on the tech side first |
elliotdavies
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall approach looks great to me
| @@ -0,0 +1,84 @@ | |||
| const React = require("react"); | |||
| const ReactMarkdown = require("react-markdown"); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we've discussed as a team what our default prettier settings should be – the EB version (i.e. single quotes, trailing commas) or the tool defaults? Either way we should maybe add a .pretterrc to this repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I totally meant to add one - I'll do that
| const ReactMarkdown = require("react-markdown"); | ||
|
|
||
| const capitalise = string => { | ||
| if (string === "aws") return string.toUpperCase(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of future-proofing, this could be an array of names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good shout, will do
| @@ -0,0 +1,19 @@ | |||
| <html> | |||
| <head> | |||
| <title>Learning!</title> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| name: a, | ||
| content, | ||
| headline: content.match(/# \[(.*?)\]/)[1], | ||
| dateAdded: fs.statSync(`${path}${a}`).birthtime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
birthtime 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seemed the most appropriate from this list
| if (diff === 0) return diff; | ||
| return diff > 0 ? -1 : 1; | ||
| }) | ||
| .splice(0, 3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine, honestly, but technically splice is a mutation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn, I think I meant slice
| @@ -0,0 +1,282 @@ | |||
| html { | |||
| font-size: 62.5%; | |||
| text-rendering: optimizeLegibility; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this? Looks cool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I googled the best way to do full cross browser font smoothing. I suspect this covers a weird Windows case or something similar
| div.wrapper { | ||
| } | ||
|
|
||
| @media screen and (min-width: 737px) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same media query as the one above?
| display: block; | ||
| border-radius: 4px; | ||
| box-shadow: 0px 1px 1px #888; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth using SASS in this file, I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - I didn't really want to have to build a whole SASS thing, but now I've written it I think it's worth the investment
This PR adds:
index.htmlfile, which we can host on GitHub pagesREADMEin favour of the static site