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

Adding Search functionality to EPIC Guide #59

Closed
wants to merge 22 commits into from
Closed

Conversation

vinitra
Copy link
Collaborator

@vinitra vinitra commented Mar 1, 2024

Resolves #53, with a simple custom google toolbar, tied to the [email protected] address.

@basil-conto and I explored a couple Jekyll-based search options but they were hard to get working; this solution seems a lot easier and nicer!

@vinitra vinitra requested review from SolalPirelli and shardulc March 1, 2024 09:59
_middle/well-being.md Outdated Show resolved Hide resolved
search.json Outdated Show resolved Hide resolved
@SolalPirelli
Copy link
Collaborator

Nice! IMHO it'd be easier to put it on the home page rather than create another top-level category just for this (that may also mess up mobile layout),

@SolalPirelli
Copy link
Collaborator

btw, github doesn't recognize "addresses" in your PR description https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

@vinitra
Copy link
Collaborator Author

vinitra commented Mar 1, 2024

Nice! IMHO it'd be easier to put it on the home page rather than create another top-level category just for this (that may also mess up mobile layout),

I think the best solution is to add the searchbar to the header directly instead of a page, but I wasn't sure if that would mess up the formatting. I could add it to the main index page but I think it won't be found easily if you're already looking at any subpage...

Update: Discussed offline with Solal, it looks better adding it to the main page!

@vinitra vinitra temporarily deployed to github-pages March 1, 2024 11:29 — with GitHub Pages Inactive
@vinitra vinitra temporarily deployed to github-pages March 1, 2024 11:30 — with GitHub Pages Inactive
Copy link
Contributor

@basil-conto basil-conto left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

_config.yml Outdated Show resolved Hide resolved
_includes/google-search.html Outdated Show resolved Hide resolved
_includes/google-search.html Outdated Show resolved Hide resolved
_includes/google-search.html Show resolved Hide resolved
_includes/google-search.html Show resolved Hide resolved
_includes/google-search.html Outdated Show resolved Hide resolved
@shardulc
Copy link
Collaborator

shardulc commented Mar 4, 2024

What's the status on this? @vinitra do you want to address Basil's comments (and any other comments from Solal or Basil IRL since I haven't been following the conversation) before another round of review?

@SolalPirelli
Copy link
Collaborator

Since it works, and I want to try removing the Google dependency soon-ish, perhaps we could merge & I'll handle Basil's nits at the same time? (and if I fail to make it work without Google, I'll just put up a PR for the nits..)

@shardulc
Copy link
Collaborator

shardulc commented Mar 4, 2024

I'd say you should make a branch off of vini/search and make your non-Google edits there. And if the nits can be easily fixed, just push directly to vini/search and I'll take care of the merge (looks like some cleanup will be required).

@vinitra
Copy link
Collaborator Author

vinitra commented Mar 5, 2024

Taking a look now!

@vinitra vinitra temporarily deployed to github-pages March 5, 2024 14:17 — with GitHub Pages Inactive
@vinitra vinitra temporarily deployed to github-pages March 5, 2024 14:19 — with GitHub Pages Inactive
@vinitra vinitra temporarily deployed to github-pages March 5, 2024 14:20 — with GitHub Pages Inactive
@vinitra vinitra temporarily deployed to github-pages March 5, 2024 14:27 — with GitHub Pages Inactive
@vinitra
Copy link
Collaborator Author

vinitra commented Mar 5, 2024

All good from my side, thanks for the comments @basil-conto!

@shardulc
Copy link
Collaborator

shardulc commented Mar 7, 2024

image

Sorry for being a bit picky, but I think we should make this work slightly better before we merge. The table on top has poor alignment and layout. (And I think the borders shouldn’t be visible anyway, even for the search box on the front page.) Also note that the font has reverted to Arial, and the sizes are set in absolute px terms, unlike what the rest of the website uses. Not sure why the default styling makes these poor choices. But looks like the official documentation has plenty of details on how to fix this!

I’m assuming you used the PSE Control Panel for this, logging in with your personal account? Is it possible to make it with an EPIC account so it’s easier to change/maintain going forward?

@vinitra
Copy link
Collaborator Author

vinitra commented Mar 12, 2024

@shardulc it is indeed using an [email protected] account as I mentioned before (on slack!) :)

@shardulc
Copy link
Collaborator

Thanks again for your work on this, Vini! Superceded by #64 but Solal benefitted from your work.

@shardulc shardulc closed this Apr 11, 2024
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.

Implement search for the EPIC Guide
4 participants