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 pinning featured posts option #353

Merged
merged 1 commit into from
Jul 27, 2022

Conversation

LukeHong
Copy link
Contributor

This PR...

Changes / fixes

  • Add options for pinning featured posts when listing

Screenshots (if applicable)

difference

Checklist

Ensure you have checked off the following before submitting your PR.

@LukeHong LukeHong force-pushed the pin_featured_posts branch 2 times, most recently from dd8eee5 to c335778 Compare July 25, 2022 19:01
@rootwork
Copy link
Contributor

Looks like a good feature. Same as the other PR, haven't tested locally yet but approving the test builds.

@LukeHong LukeHong force-pushed the pin_featured_posts branch 2 times, most recently from 9145c86 to 2c4bfaf Compare July 26, 2022 10:13
@LukeHong
Copy link
Contributor Author

The last push is to fix the variable name style

@LukeHong LukeHong force-pushed the pin_featured_posts branch from 2c4bfaf to 36c3227 Compare July 26, 2022 19:17
@LukeHong
Copy link
Contributor Author

Fix missing signoff issue, should be all good after being approved

@rootwork
Copy link
Contributor

rootwork commented Jul 26, 2022

[Edit: Nevermind about what I posted earlier; it was an error on my end.]

I would suggest changing the comment for numberOfPinnedPosts to read something like:

# Maximum number of pinned featured posts (default: 8)

since as I read the code it looks like this param doesn't need to be set in order for the pinning to work (which is good).

Also, somewhat tangentially related to this PR, I noticed that we have a "featured" tag, but that posts are only featured when featured: true is set -- it has nothing to do with the tag. We might want to address this, or at least document it, in a future PR [Edit: Opened an issue for discussion, #356].

@rootwork
Copy link
Contributor

Tested and confirmed that this works; sorry about the false negative I reported in the previous comment.

See my suggestion about the comment; otherwise this looks good.

Thanks for contributing @LukeHong!

Signed-off-by: Luke Hong <luke0724@hotmail.com>
@LukeHong LukeHong force-pushed the pin_featured_posts branch from 36c3227 to e1d52af Compare July 27, 2022 08:25
@LukeHong
Copy link
Contributor Author

@rootwork I also add it to the comment for numberOfFeaturedPosts and numberOfRecentPosts which also have a default value in code

@rootwork
Copy link
Contributor

Looks great! And you're right, good catch. Going to merge.

Thanks for this contribution as well @LukeHong!

@rootwork rootwork merged commit 19a6996 into chipzoller:master Jul 27, 2022
@CyberPunkie
Copy link

Above does not seem to work. Using latest Hugo and Clarity:

  1. featured: true
    It makes the post appear in "Featured" section, but removes it from "Recent"

  2. featured: false
    It makes the post appear in "recent" section, but removes it from "Featured"

  3. featured: true + tag:featured
    It makes the post appear in featured section BUT it is not pinned at the top of the post list.

Am I missing anything?

@LukeHong
Copy link
Contributor Author

Replied at #451 .

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.

3 participants