Skip to content

Conversation

@Hetarth02
Copy link
Contributor

This PR aims to close Issue #2141.

@mortenpi mortenpi added Type: Enhancement Format: HTML Related to the default HTML output labels Jul 5, 2023
@mortenpi mortenpi added this to the 1.0.0 milestone Jul 5, 2023
@mortenpi mortenpi changed the title - Changed LunrJs to MinisearchJs for client-side search Change from LunrJs to MinisearchJs for client-side search Jul 5, 2023
@mortenpi
Copy link
Member

mortenpi commented Jul 9, 2023

Something I noticed while testing is that it likes to repeat page headings a lot:

image

The lunr based approach doesn't do that:

image


One other note: the PR has also a lot of non-substantial formatting changes. While I don't disagree with the changes, it's generally a good idea to make the formatting changes independently. In this case, though, it's a little less important -- we're kinda replacing the whole search.js anyway. But it is a little tricky to see right now how the implementations differ, and where they're the same.

What formatter are you using though? We should probably add a config file and a CI job to format all the JS and CSS at least. There is a suggestion to use eslint: #2072. Separately, we should also apply JuliaFormatter to the Julia code too, but let's cross that bridge a bit later.

@Hetarth02
Copy link
Contributor Author

Hetarth02 commented Jul 16, 2023

@mortenpi I have fixed the duplication issue.

Additionally, I have updated the search result UI to provide better context for the results like displaying not only title but the matched string(if found) and the category badge too.

The new UI looks like this,

image

image

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

Awesome! Mostly just a bit of bikeshedding. The main thing would be moving the search layout stuff into a shared SCSS file, to avoid duplication, but then I'd pretty much be happy to merge I think.

@Hetarth02
Copy link
Contributor Author

Alright, will clean up the code as suggested.

- Cleaned variable names, now they make more sense
- Updated CHANGELOG
Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

This LGTM, thank you @Hetarth02! I need to get the Prettier workflow working properly, so I'll hold off on merging until that passes.

@mortenpi mortenpi enabled auto-merge (squash) August 4, 2023 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Format: HTML Related to the default HTML output Type: Enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants