feature: Add RSS feed support for news#198
Conversation
GAsplund
left a comment
GAsplund
left a comment
There was a problem hiding this comment.
Thank you for this PR! I think the button location is fine where it is now, but maybe @Sus-H can come with some more useful insights on that.
I have added some comments on the code implementation that will need addressing before merging, please check them out!
| @@ -0,0 +1,28 @@ | |||
| 'use client'; | |||
There was a problem hiding this comment.
It seems this component does not need any client-side functionality?
| const l = i18nService.getLocale(locale); | ||
|
|
||
| return ( | ||
| <a |
| static generateFeed( | ||
| newsItems: NewsItem[], | ||
| locale: string = 'sv', | ||
| baseUrl: string = 'https://chalmers.it' |
There was a problem hiding this comment.
This should probably not be a parameter, instead just read it from the environment variable inside the function. I don't think there's much reason to be able to make this URL different from the env-configured one?
| /** | ||
| * Converts a date to RFC 822 format for RSS feeds | ||
| * Example: "Mon, 14 Apr 2025 10:00:00 GMT" | ||
| */ | ||
| private static dateToRFC822(date: Date | string): string { | ||
| const d = typeof date === 'string' ? new Date(date) : date; | ||
| const days = ['Sun', 'Mon', 'Tue', 'Wed', 'Thu', 'Fri', 'Sat']; | ||
| const months = [ | ||
| 'Jan', | ||
| 'Feb', | ||
| 'Mar', | ||
| 'Apr', | ||
| 'May', | ||
| 'Jun', | ||
| 'Jul', | ||
| 'Aug', | ||
| 'Sep', | ||
| 'Oct', | ||
| 'Nov', | ||
| 'Dec' | ||
| ]; | ||
|
|
||
| const dayName = days[d.getUTCDay()]; | ||
| const dayNum = String(d.getUTCDate()).padStart(2, '0'); | ||
| const monthName = months[d.getUTCMonth()]; | ||
| const year = d.getUTCFullYear(); | ||
| const hours = String(d.getUTCHours()).padStart(2, '0'); | ||
| const minutes = String(d.getUTCMinutes()).padStart(2, '0'); | ||
| const seconds = String(d.getUTCSeconds()).padStart(2, '0'); | ||
|
|
||
| return `${dayName}, ${dayNum} ${monthName} ${year} ${hours}:${minutes}:${seconds} GMT`; | ||
| } |
There was a problem hiding this comment.
Use Date.toUTCString instead. RFC-7231 is compatible with RFC-822.
| padding: 0.5rem; | ||
| border-radius: 0.25rem; |
There was a problem hiding this comment.
Nitpick: You can probably remove this with no regressions.
There was a problem hiding this comment.
Not quite true I believe. Having a padding is important for mobile, and the border-radius is just a bit of eye-candy for the tab-focused state
|
Made a commit based on your feedback! |
|
Looks better! Don't forget to remove the old RFC822 function and the (now) extra parameter. |
|
I think it looks good. Assuming this is a feature for advanced users, it's good that it somewhat blends in. I would maybe experiment with vertical middle align, baseline align, or have the icon be the same as the line height. |
3 participants
The purpose of this PR is to add a "Subscribe via RSS" button for the news list. I have implemented the RSS feed via an /api route, supporting both sv and en locales.
However, I'd like to get feedback on the UI part of this PR. I have currently placed the button next to the "Nyheter" ("News") heading on the home page, and I'm not sure how to get it to look good.