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

Eleventy refactor #206

Merged
merged 35 commits into from
Jul 14, 2020
Merged

Eleventy refactor #206

merged 35 commits into from
Jul 14, 2020

Conversation

emersonthis
Copy link

@emersonthis emersonthis commented Jun 17, 2020

Overview

Converts the site to Eleventy (instead of Jekyll)

Closes #159
Closes #208
Closes #209
Closes #207

Copy link
Member

@tylersticka tylersticka left a comment

Choose a reason for hiding this comment

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

The Netlify deploy preview says that it succeeded, but the URL doesn't actually work. We cannot merge until this is resolved.

netlify.toml Outdated Show resolved Hide resolved
.eleventy.js Outdated Show resolved Hide resolved
.eleventyignore Outdated Show resolved Hide resolved
.nvmrc Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
_includes/head.html Outdated Show resolved Hide resolved
---

<div class="u-containProse u-md-textGrow1">
{% include resource-item.html,
Copy link
Member

Choose a reason for hiding this comment

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

These files are all using the .html extension, but I notice these {% tags. What template engine is being used? Is there a reason we aren't using its file extension so syntax highlighting will be easier?

Copy link
Author

Choose a reason for hiding this comment

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

Eleventy's default template language for HTML is liquid, so that's what's being used here. These are the same extensions leftover from the Jekyll version, so no strategic choice was made (by me) to remove them. I don't think Eleventy will care if we add them, but it will add a good bit of noise to the diff for this PR. Maybe easier to do that in a separate PR?

Copy link
Author

Choose a reason for hiding this comment

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

@emersonthis
Copy link
Author

I'm used to that pattern for pagination URLs so it didn't stick out for me. But I'll look into overriding that. What would the perfect path be if you could choose anything? /page/#? Or did you want to replicate the /page1, /page2 from the Jekyll version?

@emersonthis
Copy link
Author

Confirmed. Eleventy pagination permalinks are easy to override in the same way as Jekyll.

While fiddling with this, I noticed that there's no pagination for the tag archive pages. (It just shows them all on one page) This is hard to spot because there aren't enough of most tags to overflow the first page. But a popular tag like Service Worker has 20. I'm not sure if this is a show-stopper, so I'll create an issue to track this bug in case we decide to merge before it's fixed.

@tylersticka
Copy link
Member

I'm used to that pattern for pagination URLs so it didn't stick out for me. But I'll look into overriding that. What would the perfect path be if you could choose anything? /page/#? Or did you want to replicate the /page1, /page2 from the Jekyll version?

I don't have a strong opinion, I mainly just don't want to break links if we don't have to. I know pagination links are a poor choice for sharing URLs, but that doesn't mean no one did.

Making the structure the same as Jekyll seemed like the simplest way to do that, but if you or others would prefer a different format, just make sure to add Netlify rules for redirecting from the old structure to the new one: https://docs.netlify.com/routing/redirects/

@emersonthis
Copy link
Author

Pagination links are updated to match Jekyll.

@tylersticka
Copy link
Member

Pagination links are updated to match Jekyll.

Did you push this commit? I don't see any changes.

@emersonthis
Copy link
Author

Apparently I didn't! But now it's true...

Copy link
Member

@tylersticka tylersticka left a comment

Choose a reason for hiding this comment

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

Works great locally! Some nits to pick inline.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
_includes/layouts/blank.html Outdated Show resolved Hide resolved
@tylersticka
Copy link
Member

@emersonthis It looks like we haven't finished porting over the RSS feed? This is necessary for our social integrations.

Live site: https://www.pwastats.com/feed.xml

404 in deploy preview: https://deploy-preview-206--pwastats.netlify.app/feed.xml

@emersonthis
Copy link
Author

I didn't notice there was an RSS feed until now. It looks like that functionality was provided by a Jekyll plugin. There appears to be an analogous plugin for Eleventy which looks easy enough to install...

@emersonthis
Copy link
Author

@tylersticka I reimplemented the rss feed. The new plugin uses Nunjucks syntax by default. This didn't seem problematic to me, but I'm flagging it because I have a hunch this might bother you. If so, I recommend we tackle that as a follow-up issue (instead of pre-merge) because there are some rss-specific filters that I don't think will work automatically in liquid.

Also of note: there are minor changes to the generated XML data. I'm not an expert on RSS format/validity but according to this validator, the new version is valid. Interestingly, the Jekyll version does not pass.

@tylersticka
Copy link
Member

This didn't seem problematic to me, but I'm flagging it because I have a hunch this might bother you.

I prefer Nunjucks to Liquid so it doesn't bother me.

@tylersticka
Copy link
Member

Screen Shot 2020-07-08 at 9 36 30 AM

@emersonthis Appears to be some sort of pagination regression in the current version.

@emersonthis
Copy link
Author

@tylersticka I checked the preview site and I did see the glitch in your screenshot once. Then I clicked to a different page and I haven't been able to reproduce it since. Are you able to reproduce it?

I have a hunch at what happened: We use the purgecss script to weed out all unused CSS rules from the pattern library. Before that commit, there were no styles to position the arrows (because they didn't appear anywhere in the site). When I first added the pagination partial in my local environment, I had to re-run eleventy so that purgecss would find the arrows and include the corresponding styles. I suspect caching may have caused a similar hiccup on the staging site.

@emersonthis
Copy link
Author

All of Megan's QA issues have been addressed, and RSS/twitter due diligence discussed in Slack is complete.

@emersonthis
Copy link
Author

@megnotarte @tylersticka Megan's QA issues have been closed and I believe all of Tyler's prior feedback has been addressed. I think this is once again on the launchpad...

Copy link
Member

@tylersticka tylersticka left a comment

Choose a reason for hiding this comment

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

I believe @megnotarte and I have done all the reviewing we can, and things appear to be working as expected! Hooray!

Please note that merging this PR will deploy these changes live. I'd wait to merge until you have some time to watch the Twitter bot and react to any unforeseen regressions that may occur.

@emersonthis emersonthis merged commit f98518f into master Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants