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

Website: Drop minima theme, fix RSS feeds #641

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

fulldecent
Copy link
Contributor

@fulldecent fulldecent commented Sep 15, 2024

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).

Screenshot 2024-09-15 at 17 53 35


Screenshot 2024-09-15 at 17 53 41


Screenshot 2024-09-15 at 17 53 49

@eip-review-bot
Copy link
Collaborator

eip-review-bot commented Sep 15, 2024

File 404.html

Requires 3 more reviewers from @axic, @g11tech, @gcolvin, @lightclient, @SamWilsn, @xinbenlv

File Gemfile

Requires 3 more reviewers from @axic, @g11tech, @gcolvin, @lightclient, @SamWilsn, @xinbenlv

File Gemfile.lock

Requires 3 more reviewers from @axic, @g11tech, @gcolvin, @lightclient, @SamWilsn, @xinbenlv

File _config.yml

Requires 3 more reviewers from @axic, @g11tech, @gcolvin, @lightclient, @SamWilsn, @xinbenlv

File _includes/eiptable.html

Requires 3 more reviewers from @axic, @g11tech, @gcolvin, @lightclient, @SamWilsn, @xinbenlv

File _includes/footer.html

Requires 3 more reviewers from @axic, @g11tech, @gcolvin, @lightclient, @SamWilsn, @xinbenlv

File _includes/head.html

Requires 3 more reviewers from @axic, @g11tech, @gcolvin, @lightclient, @SamWilsn, @xinbenlv

File _includes/header.html

Requires 3 more reviewers from @axic, @g11tech, @gcolvin, @lightclient, @SamWilsn, @xinbenlv

File _includes/social.html

Requires 3 more reviewers from @axic, @g11tech, @gcolvin, @lightclient, @SamWilsn, @xinbenlv

File _layouts/base.html

Requires 3 more reviewers from @axic, @g11tech, @gcolvin, @lightclient, @SamWilsn, @xinbenlv

File _layouts/eip.html

Requires 3 more reviewers from @axic, @g11tech, @gcolvin, @lightclient, @SamWilsn, @xinbenlv

File _layouts/page.html

Requires 3 more reviewers from @axic, @g11tech, @gcolvin, @lightclient, @SamWilsn, @xinbenlv

File assets/css/style.scss

Requires 3 more reviewers from @axic, @g11tech, @gcolvin, @lightclient, @SamWilsn, @xinbenlv

File index.html

Requires 3 more reviewers from @axic, @g11tech, @gcolvin, @lightclient, @SamWilsn, @xinbenlv

@eip-review-bot eip-review-bot changed the title Drop minima theme, fix RSS feeds Website: Drop minima theme, fix RSS feeds Sep 15, 2024
@SamWilsn
Copy link
Collaborator

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!

@fulldecent
Copy link
Contributor Author

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.

@fulldecent
Copy link
Contributor Author

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 }}" />
Copy link
Collaborator

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?

Copy link
Contributor Author

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"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should remove this too?

Copy link
Contributor Author

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.

@SamWilsn
Copy link
Collaborator

Hello hello. Can I please get a merge on this PR?

To see my current review queue, take a look at EIPs Insight.

@fulldecent
Copy link
Contributor Author

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. :-)

@xinbenlv
Copy link
Collaborator

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?

@fulldecent
Copy link
Contributor Author

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

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.

4 participants