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

Add titles-only RSS feed #47

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

Conversation

kevinlitchfield
Copy link

Adds a titles-only RSS feed accessible at /rss-titles to complement the full RSS feed available at /rss.

Sidebar now looks like this:

screen shot 2017-03-24 at 1 46 37 pm

Addresses #45.

Copy link
Owner

@rrrene rrrene left a comment

Choose a reason for hiding this comment

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

Thx! Please find comments attached.

@@ -11,8 +11,13 @@ defmodule ElixirStatus.FeedController do
|> send_file(200, "priv/static/images/github/#{user_name}.jpg")
end

def rss(conn, _params) do
def full_feed(conn, _params) do
Copy link
Owner

Choose a reason for hiding this comment

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

Please revert all changes to this function.

@@ -23,7 +23,8 @@
<span class="sidebar-nav-item sidebar-nav-item--text"><%= gettext "You can follow via" %></span>
<a class="sidebar-nav-item no-hover-decoration" href="http://elixirweekly.net/" target="_blank"><span class="badge badge--new">NEW!</span> <span class="hover-decoration">ElixirWeekly</span></a>
<a class="sidebar-nav-item" href="https://twitter.com/elixirstatus" target="_blank">Twitter</a>
<a class="sidebar-nav-item" href="/rss">RSS</a>
<a class="sidebar-nav-item" href="/rss">RSS (full)</a>
Copy link
Owner

Choose a reason for hiding this comment

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

Please revert all changes to this file.

postings = Posting.published
render(conn, "rss.xml", postings: postings.entries)
render(conn, "full.xml", postings: postings.entries)
Copy link
Owner

Choose a reason for hiding this comment

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

Please name all templates like their functions.

render(conn, "full.xml", postings: postings.entries)
end

def titles_feed(conn, _params) do
Copy link
Owner

Choose a reason for hiding this comment

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

Please rename titles_feed to rss_titles_only.

web/router.ex Outdated
@@ -39,7 +39,9 @@ defmodule ElixirStatus.Router do
get "/", PostingController, :index
get "/u/:user_name", PostingController, :user

get "/rss", FeedController, :rss
get "/rss", FeedController, :full_feed
get "/rss-titles", FeedController, :titles_feed
Copy link
Owner

Choose a reason for hiding this comment

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

Please rename rss-titles to rss-titles-only


def titles_feed(conn, _params) do
postings = Posting.published
render(conn, "titles.xml", postings: postings.entries)
Copy link
Owner

Choose a reason for hiding this comment

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

Please rename rss_titles_only.xml.

@kevinlitchfield
Copy link
Author

@rrrene Thanks for the review, and apologies for the delay. Have addressed all feedback.

Since the titles-only RSS feed is no longer linked anywhere, it's not discoverable by browsing the site. Should it be linked somewhere or no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants