-
-
Notifications
You must be signed in to change notification settings - Fork 68
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
feature: upgrade rss behaviour #347
feature: upgrade rss behaviour #347
Conversation
@linkdotnet I'm having trouble with debugging during development—neither the project nor the tests will debug, and it just silently stops. I'm using Rider on Linux. Any ideas on what I might be missing? |
I do think that is a short-coming from Rider with the current Might be worth to open a issue (it seems to be related to Blazor, because my blog is the only project I can't debug ATM). |
I also have the same issue if I just create a new xUnit project with net9.0. |
I would call it "Compact" and "Full". We can refer to the readme and add a section there. |
Maybe I missunderstood, wasn't it supposed to be a limit (following |
Fair point - yes. That doesn't make the wording easier though.
Something like that? |
I like that! What should we do with the icons inside the dropdown? Stay or go? |
I would say "Bye Bye" given that we already have an icon for the header. |
If you wanna weigh in |
@elementh As a heads up: I migrated all tests from FluentAssertions to Shouldly. Also I enabled nullability (that was missing until now for all tests). You might want to rebase/merge from the current master. |
@linkdotnet sounds great, will do. I've been trying to do the markup thing as clean as possible, but I run into some problems with the The old "simple" workarounds don't work anymore, so I have to come up with something else. There is something that would probably work, but it involves creating the feed and then manually replacing content, and that sounds so bad hahaha |
new test expects that rss returns full blog post content when requested
if requested, rss will return full content for the blog posts, as well as limit the number of returned posts to the BlogPostsPerPage configuration
when also requesting the content
6175e66
to
d004ba0
Compare
The force push is for the rebase, sorry. Okay, everything should be ready and working, please have a look and let me know! |
I do that all the time. I am "Camp Rebase" ;)
Could you also add Rate-Limiting for the Controller? In any case - big thanks for the support. I will have a look. |
Also, checking the feed output with a validator (https://validator.w3.org/feed/check.cgi) tells me that the image element is unknown, and on my RSS Reader I see no images from your blog posts. Should we make it a |
Should we use https://learn.microsoft.com/en-us/aspnet/core/performance/rate-limit or do you prefer any third party packages? |
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.
Really good job! A few changes and we are there!
tests/LinkDotNet.Blog.IntegrationTests/Web/Controller/RssFeedControllerTests.cs
Show resolved
Hide resolved
I would go with the ASP.NET one as you proposed. If you feel very motivated you can even use the |
to GetBlogPostsItemsWithContent where it is actually used
Hmmm I initially found that on Stackoverflow. Have to check that with an RSS Feed Reader. If you can confirm that "img" works - go ahead. |
On feedly it works with the current setup |
when the numberOfBlogPosts is not specified in the call (RSS)
Let's leave it like that then, at least for now! |
I'll do this either this afternoon or tomorrow! |
tests/LinkDotNet.Blog.IntegrationTests/Web/Controller/RssFeedControllerTests.cs
Show resolved
Hide resolved
We can also do this in a follow up PR if you wish |
Sounds great, should one of us open a new issue for the rate limiting too? |
I would keep it open. Just split the feature into 2 sep. PR's |
Even better 😄 |
Progress:
BlogPostsPerPage
Will close #343