Skip to content

Conversation

@ash-jc-allen
Copy link
Contributor

Hey!

This PR proposes a small change that will include pinned articles in the regular articles list if there are more than 4 pinned articles.

For a bit of context, I have 19 articles published on laravel.io. However, you can only actually see 18 (or at least that's all I can see haha). One of them is in the pined list at the top. The other 17 are in the regular articles list. From what I can gather, I think it's because this article is currently pinned: https://laravel.io/articles/how-to-add-breadcrumbs-to-your-laravel-website

So from my guess, I think this article will be in a sort of "limbo" state. I don't think it'll get picked up by the pinned articles query because there are newer pinned articles. I also don't think it'll get picked up by the regular articles query because the article is is_pinned.

With my new change, it proposes that if a pinned article isn't included in the 4 pinned articles, it'll then be included int the regular article listing.

I'm hoping what I've done is right (and actually needed/wanted haha!). If this is something you do think might be worth merging, I'd be more than happy to make any changes if needed 😄

I've added some tests to make sure it works programmatically. But I've not done any manual testing because my local env is a little bit broken at the moment haha! So I do apologise if I've done anything wrong or broken anything!

@joedixon
Copy link
Contributor

Thanks @ash-jc-allen this is a really great catch.

However, I wonder if this is the correct approach as the pinned articles will only ever be the first pulled from the database with the is_pinned flag.

Perhaps we should, instead, only allow n articles to be pinned at any one time?

What do you think @ash-jc-allen and @driesvints?

@ash-jc-allen
Copy link
Contributor Author

That's a great point! I did consider using that type of approach but wasn't too sure what you guys would think.

If you think that'd be a better approach, I'm more than happy to close this PR and then make the changes in a separate branch 🙂

@joedixon
Copy link
Contributor

Let's see what @driesvints thinks when he gets a chance to check in

@driesvints
Copy link
Member

Hey all. Thanks @ash-jc-allen, this is a great catch. I do agree with @joedixon though to limit pinned articles to max 4. Can we make that change instead?

@ash-jc-allen
Copy link
Contributor Author

Hey @driesvints, that's no problem at all! I've had a crack at making these changes in #943 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants