-
Notifications
You must be signed in to change notification settings - Fork 457
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
Website: Drop minima theme, fix RSS feeds #641
base: master
Are you sure you want to change the base?
Conversation
File
|
Thank you for taking a look at this! I'll try to review this and merge it at some point. Is there a matching PR for EIPs? If not, don't worry about making one. I'll take care of it. I am (very slowly) working on moving to Zola for rendering EIPs/ERCs. I'm not much of an HTML expert, but I have a theme that replicates the look of the current website. If you're interested, I'd love some help over there too! |
I will need a slightly different fix for EIP repo. And I will do that. But I need some momentum on my PRs getting merged to continue on this. |
Hello hello. Can I please get a merge on this PR? |
{%- feed_meta -%} | ||
<link type="application/atom+xml" rel="alternate" href="{{ '/rss/all.xml' | relative_url }}" title="{{ site.title | escape }}" /> |
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.
I don't know Jekyll at all. Can you explain?
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.
The line that is being removed is a bunch of magic for generating RSS, which is not the RSS we want.
The line that is being added is an explicit RSS feed that makes sense for this repo.
Gemfile
Outdated
@@ -10,10 +10,7 @@ source "https://rubygems.org" | |||
# This is the default theme for new Jekyll sites. You may change this to anything you like. | |||
gem "minima", "~> 2.0" |
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.
Should remove this too?
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.
Yes, removing the line has no impact. Removed in a new commit.
To see my current review queue, take a look at EIPs Insight. |
A merge on this PR here directly increases visibility of our EIPs for reviewers (with RSS and email notification) thus decimating work in your queue. :-) |
I think this is a great direction. I am in favor of this PR to be merged. That said, is there a link we can try it? |
Sorry we didn't set up webpage builds here for PRs. But I did set up screenshots above, it works on responsive. And if there is something broken (even if I didn't break) you can see I'm quick to maintain things |
The work for this PR is to fix the RSS feeds on the ERCs website.
The problem was that the Jekyll Minima theme interferes with any custom RSS feed. (We use a custom RSS feed because people care about our ERCs, not changes to our other pages.) And therefore it was necessary for me to remove the Minima theme (and bump gem deps) to achieve this.
In this process I took the minor liberty of using the Bootstrap 5 CSS (which we were already using) to renter the header and footer rather than recreating dozens of Minima rules that we were previously relying on (and again which were duplicative to Bootstrap 5).
I did NOT take the liberty of fixing the header or footer, or all kinds of other problems with the site. I would like to do so, but that is not part of this minimal PR.
Here is what the page looks like. So you can see the slight update in style which is a side effect of this PR. And you can see the now working RSS feeds. Also the metadata on the pages linking to the RSS is fixed (and not visible in the screenshots).