Skip to content
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

Render overview on the index route #35

Merged
merged 5 commits into from
Dec 11, 2017
Merged

Render overview on the index route #35

merged 5 commits into from
Dec 11, 2017

Conversation

safinn
Copy link
Contributor

@safinn safinn commented Dec 8, 2017

Solves #13

Renders an overview page using handlebars.
Had to add the size of the files in the cache to be able to display them.

Added a few packages:

  • timeago.js to convert the publish date.
  • handlebars to render the page.

screen shot 2017-12-08 at 22 43 47

@leo
Copy link
Contributor

leo commented Dec 11, 2017

Amazing! Looks great.

Could you please make the function for rendering the handlebars template asynchronous? In addition, I don't see a need for including fs-extra, you can just use promsify(fs.readFile) (from the native "util" module).

lib/view.js Outdated
let viewContent = false
const viewPath = path.normalize(path.join(__dirname, '/../views/index.hbs'))

try {
viewContent = fs.readFileSync(viewPath, 'utf8')
viewContent = await readFileAsync(viewPath, { encoding: 'utf8' })
Copy link
Contributor

Choose a reason for hiding this comment

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

'utf8' works even with the native module 😊 no need for an object

Copy link
Contributor

@leo leo left a comment

Choose a reason for hiding this comment

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

Thanks for your changes! I just found a last few bits to improve, but then we're good. Thank you! 😊

lib/view.js Outdated
try {
viewContent = await readFileAsync(viewPath, 'utf8')
} catch (err) {
throw err
Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't handle the error, you can just write:

const viewContent = await readFileAsync(viewPath, 'utf8')

Without the try/catch block 😊

Copy link
Contributor

Choose a reason for hiding this comment

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

This also means you can remove the let viewContent = false at the top... 🚀

lib/routes.js Outdated
const details = {
account: config.account,
repository: config.repository,
date: timeago().format(cache.latest.pub_date),
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use date-fns here please 😊 It's faster, slimmer and currently the best option out there.

views/index.hbs Outdated
background: linear-gradient(#ff00a5, #f65538)
}

#releaseNotes {
Copy link
Contributor

Choose a reason for hiding this comment

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

No camelcase in CSS properties please, always dashes (also adjust the HTML) 😊

@leo leo changed the title Show overview page at the home url Render overview on the index route Dec 11, 2017
@leo leo merged commit 31ee3ac into vercel:master Dec 11, 2017
@leo
Copy link
Contributor

leo commented Dec 11, 2017

Thank you for this wonderful PR! 😊

@safinn
Copy link
Contributor Author

safinn commented Dec 11, 2017

No worries. Learnt about date-fns. Looks really good! 👍

@safinn safinn deleted the overview branch December 11, 2017 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants