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

Added sort by default to nodes #58

Merged
merged 3 commits into from
Oct 18, 2022
Merged

Added sort by default to nodes #58

merged 3 commits into from
Oct 18, 2022

Conversation

jessems
Copy link
Contributor

@jessems jessems commented Oct 15, 2022

By default the nodes are sorted by id, not by title or relativePath. This is a bit counter intuitive when you have a bunch of markdown files and folders with alphabetic sorting (e.g. "01 this is the first file" and "02 this is the second file") and they don't appear in the same sequence in the sidebar menu.

This little sort fixes that.
image

Copy link
Owner

@hikerpig hikerpig left a comment

Choose a reason for hiding this comment

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

Thank you for this nice enhancement.
I've leave one comment regard of the code.

@hikerpig
Copy link
Owner

hikerpig commented Oct 17, 2022

It occurred to me that sorting by relativePath will also cause some confusions because sidebar displays article title rather than file path. So it sometimes will look strange if file path and title are not the same.
I think it'd be better if the sorting can be configured, but this PR itself is good enough to be merged (with the problem in my CR comment fixed of course. It's a good default behavior this way. And I will make it configurable later.
image
image

@jessems
Copy link
Contributor Author

jessems commented Oct 17, 2022

Updated the pull request to sort on validNodes. And I agree with you, having the option to sort on title would be even better. I ran into a case where I needed it already.

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.

2 participants