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

Sort by a new 'priority' parameter #26

Closed
wants to merge 3 commits into from
Closed

Sort by a new 'priority' parameter #26

wants to merge 3 commits into from

Conversation

tomap
Copy link
Contributor

@tomap tomap commented Jan 18, 2019

Bump minor, because there could be a minor change in behavior
if some user already have a priority parameter
on their posts

Based upon the work of #6

Bump minor, because there could be a minor change in behavior
if some user already have a priority parameter
on their posts
@tomap tomap mentioned this pull request Jan 18, 2019
@coveralls
Copy link

coveralls commented Jan 18, 2019

Coverage Status

Coverage decreased (-25.0%) to 75.0% when pulling 69992b0 on tomap:master into e4129ae on hexojs:master.

@YunYouJun
Copy link

I've been looking forward to this feature for a long time.
The earlier merged, the better.


By the way, I found some theme such as hexo-theme-next, hexo-theme-melody etc.
They had added sticky style.
Maybe use sticky replace priority is ok?

@stevenjoezhang
Copy link
Member

The parameter config.index_generator.order_by is ignored

@curbengh
Copy link
Contributor

curbengh commented Jul 9, 2019

The parameter config.index_generator.order_by is ignored

That's the intention.

note that if your posts have a `priority` property, it will be used to order your post taking precedence over `order_by`.

@stevenjoezhang
Copy link
Member

That's the intention.

note that if your posts have a priority property, it will be used to order your post taking precedence over order_by.

But how about posts with same priority? It does not match the description.

@tomap
Copy link
Contributor Author

tomap commented Aug 14, 2019

well, I just looked at the code again, and it is not what I intended...
I was trying to reproduce the work of #6
But what we have here is something that will break the parameter config.index_generator.order_by in all cases, which is not good

I believe the configuration config.index_generator.order_by can be used to order by priority, if a priority has been defined on all posts
What might be missing is a was to order by priority first, if there is a priority defined, it goes on top
If not, the config.index_generator.order_by is used
I'm not sure how to code that :)

@stevenjoezhang
Copy link
Member

Yes, the issue is: When two or more posts have the same priority, the sorting should fall back to config.index_generator.order_by, but not

if(a.priority == b.priority) return b.date - a.date;

@stevenjoezhang
Copy link
Member

But what we have here is something that will break the parameter config.index_generator.order_by in all cases, which is not good

@tomap I think you are talking about the stability of the sorting algorithm. Node.js 12+ uses stable sorting algorithms by default: https://v8.dev/features/stable-sort . Therefore, you can sort locals.posts by config.index_generator.order_by (e.g. '-date') first, and then sort locals.posts.data according to the value of sticky or priority parameter. For older Nodejs versions, lodash is required to implement this method.

Comment on lines +7 to +21
const posts = locals.posts;
posts.data = posts.data.sort(function(a, b) {
if(a.priority && b.priority) {
if(a.priority == b.priority) return b.date - a.date;
else return b.priority - a.priority;
}
else if(a.priority && !b.priority) {
return -1;
}
else if(!a.priority && b.priority) {
return 1;
}
else return b.date - a.date;
});

Copy link
Member

@stevenjoezhang stevenjoezhang Mar 4, 2020

Choose a reason for hiding this comment

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

Suggested change
const posts = locals.posts;
posts.data = posts.data.sort(function(a, b) {
if(a.priority && b.priority) {
if(a.priority == b.priority) return b.date - a.date;
else return b.priority - a.priority;
}
else if(a.priority && !b.priority) {
return -1;
}
else if(!a.priority && b.priority) {
return 1;
}
else return b.date - a.date;
});
const posts = locals.posts.sort(config.index_generator.order_by);
posts.data.sort((a, b) => (b.sticky || 0) - (a.sticky || 0));

Stable sorting requires Node.js 12+.

@curbengh
Copy link
Contributor

curbengh commented Mar 5, 2020

@stevenjoezhang

can you create another PR with your patch? This PR has been inactive for a while...

@SukkaW
Copy link
Member

SukkaW commented Apr 26, 2020

cc @stevenjoezhang

@stevenjoezhang
Copy link
Member

stevenjoezhang commented Apr 26, 2020

@curbengh @SukkaW Superseded by #51

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.

6 participants