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

updates default sort #927

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

updates default sort #927

wants to merge 3 commits into from

Conversation

bulbil
Copy link

@bulbil bulbil commented Apr 30, 2024

Thank you for contributing to Quire! Please complete the form below to submit your pull request for review.

For the Title of this pull request, please use the format "Type/Issue-#: Brief description." For Type, the options are Fix, Feature, Docs, or Chore. Issue-# is only needed if this pull request addresses an existing issue.

Before submitting your PR, make sure that you can run the following commands: quire preview, quire build, quire pdf, and quire epub. If any of those core quire commands throw errors/yield unexpected behavior, we will NOT review the PR!

Checklist

Please put an X within the brackets that apply [X].

  • I have read the CONTRIBUTING.md file

  • I have made my changes in a new branch and not directly in the main branch

  • This pull request is ready for final review by the Quire team

Is this pull request related to an open issue? If so, what is the issue number?

Please briefly describe the goal of this pull request and how it may impact Quire's functionality.

Developing locally I found that when order is not set or has many duplicate values, default sort was not stable.

I think Hugo's default sort fallbacks and the way it uses weight is significantly different from the current behavior. I created a useCustomSort config key to implement a more flexible TOC sort based on title string.

At the very least, I propose you getAllSorted first in order to ensure that order is not different between builds when order is missing.

Please describe the changes you made, and call out any details you think are particularly relevant for the Quire team to note in their review.

Does this pull request necessitate changes to Quire's documentation?

Include screenshots of before/after if applicable.

Additional Comments

@bulbil
Copy link
Author

bulbil commented Apr 30, 2024

@anderspollack here's that super minor update I mentioned earlier today to do with sort -- not having been part of the conversations, not sure how helpful this is or what desired behavior is.

I just thought having order be inconsistent between builds wasn't ideal. I have a more elaborate custom sort but thought that would require more discussion.

As a sidenote, I have an idea about how to have search-index.json not include HTML from the parsed MD, be happy to chat about that. It's not the most elegant thing and it doesn't seem like a pressing concern -- but in case you are interested I'd be happy to write something up.

Copy link
Member

@mphstudios mphstudios left a comment

Choose a reason for hiding this comment

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

@bulbil having the collection sort be consistent across builds is more clear, thank you for the fix! I am aware that the linter may not be functioning properly though Quire uses code format rules no-semi and single-quotes, if you update this pull-request with those formatting changes we will review/test/merge the change.

@bulbil
Copy link
Author

bulbil commented May 1, 2024

@mphstudios apologies, I realized I had updated linting in my Quire project but not in this branch.

@bulbil bulbil requested a review from mphstudios May 1, 2024 20:16
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