Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Apr 9, 2024

This adds a search functionality to the website, powered by lunr.js. While data is prepared and collected during build time, the index is built on the client, as the precomputed index is 1.2 MB, which is already the size of a full page load (even if the data is only fetched once you actually search something). I wasn't able to make this work with dynamically routed pages, although it is fully compatible with all our generated pages, as the content is fetched from the built HTML. I also haven't included pagination and mobile support, probably something for a future improvement.


See preview on Cloudflare Pages: https://preview-200.quiltmc-org.pages.dev

simpledemon added 2 commits April 9, 2024 20:07
This adds a search functionality to the website, powered by lunr.js. While data is prepared and collected during build time, the index is built on the client, as the precomputed index is 1.2 MB, which is already the size of a full page load (even if the data is only fetched once you actually search something). I wasn't able to make this work with dynamically routed pages, although it is fully compatible with all our generated pages, as the content is fetched from the built HTML. I also haven't included pagination and mobile support, probably something for a future improvement.
@FirstMegaGame4
Copy link

FirstMegaGame4 commented Apr 9, 2024

okay, that is incredible
image
shouldn't it better to put the access to the search bar at the bottom-right of the page though? with a small rounded button with the search icon i.e.

@ghost
Copy link
Author

ghost commented Apr 9, 2024

It actually highlights matching words, I just fixed that

image

Not sure about moving it, that's personally where I would expect a search bar to be

@anonymous123-code
Copy link

The contrast feels kind of low too me, and sites tend to make icon and text of the same color
image

@ix0rai ix0rai added the enhancement New feature or request label Apr 9, 2024
@ghost
Copy link
Author

ghost commented Apr 9, 2024

image

Best I can do is reenable the border. Although it passes accessibility checks anyway.

Copy link
Member

@Pyrofab Pyrofab left a comment

Choose a reason for hiding this comment

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

Seems pretty good

}

function highlight(text, startIndex, match) {
const highlightColor = (window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches) ? 'link' : 'warning';
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this the kind of thing CSS variables are made for ?

Copy link
Author

Choose a reason for hiding this comment

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

there isn't a better way to go about it without adding additional CSS to our bulma style

Copy link
Member

Choose a reason for hiding this comment

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

Is that an issue ? I feel like a "highlighted" class would be decently reusable

Copy link
Author

Choose a reason for hiding this comment

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

sure, I just can't be bothered honestly, I would have to yell at way too many people to get this done considering it needs to be both merged and published

Copy link
Member

Choose a reason for hiding this comment

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

Wait it has to go in the quilt-bulma repository ? We can't have custom styles here ?

Copy link
Author

@ghost ghost Apr 13, 2024

Choose a reason for hiding this comment

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

Nope, the CSS is built there, we don't have access to variables here. That's why I hate it.

Copy link
Contributor

Choose a reason for hiding this comment

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

given that the rest of the site has similar issues, I'd say it is fine for now and can be addressed in a future PR
(on that note I really would like a theme chooser instead of relying on the browser's preferred color scheme ONLY..)

Copy link
Member

Choose a reason for hiding this comment

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

pain²

}

function cleanPath(path: string): string {
if (process.platform === "win32") {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not simply check if it starts with /?

Copy link
Author

Choose a reason for hiding this comment

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

Linux

Copy link
Contributor

Choose a reason for hiding this comment

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

but your comment says only on windows do the paths start with an extra /?

Copy link
Author

@ghost ghost Apr 13, 2024

Choose a reason for hiding this comment

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

On windows it looks like /C:/... which is just.. wrong. But on Linux you have your normal /home/... path, if you remove the leading slash here you get a broken path as well.

It is the expected behavior on Linux but not on Windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not relativize the paths before returning them?
is that not what that dir parameter is for?

Copy link
Author

Choose a reason for hiding this comment

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

No, because the path is fucked and needs to be unfucked no matter what. This works, honestly.

}

function highlight(text, startIndex, match) {
const highlightColor = (window.matchMedia && window.matchMedia('(prefers-color-scheme: dark)').matches) ? 'link' : 'warning';
Copy link
Contributor

Choose a reason for hiding this comment

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

given that the rest of the site has similar issues, I'd say it is fine for now and can be addressed in a future PR
(on that note I really would like a theme chooser instead of relying on the browser's preferred color scheme ONLY..)

@LambdAurora LambdAurora merged commit 3114c4d into QuiltMC:main Apr 14, 2024
@ghost ghost deleted the search branch April 16, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants