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.

Sign up for GitHub

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

Jump to bottom

fix: bug in search that was indexing non-content pages #74

Merged
merged 3 commits into from
Dec 20, 2019
Merged

fix: bug in search that was indexing non-content pages #74

merged 3 commits into from
Dec 20, 2019

Conversation

bguiz
Copy link
Contributor

@bguiz bguiz commented Dec 17, 2019 •
edited
Loading

What

  • Updated template used to generated static search index JSON
    • Now whitelists content sections
    • Now removes redirect pages
    • Also strips markdown formatting and special characters
  • Updated deps in Gemfile to declare jekyll plugins properly

Why

  • There were some illegal escape characters sneaking into the rendered JSON file
  • Non-content files were also getting indexed
  • Gemfile
    • was updated in an attempt to upgrade to Jekyll 4 in order to use boolean conditions within where_exp, but that was prevented by github-pages gem
    • made jemoji rendering work properly

Refs

@bguiz bguiz requested review from a team and alepc253 and removed request for a team December 17, 2019 04:15
@alepc253
Copy link
Contributor

@bguiz I'm not sure if it is what you tried to fix, but check the "{title}":

Captura de Pantalla 2019-12-17 a la(s) 10 04 14

@bguiz
proj: fix jekyll & jekyll deps declarations in gem file; jemoji rende…
81d7d31 
…rs correctly now
@bguiz
fix: update filters to exclude redirect pages since these are missing…
d0f83eb 
… their titles

- NOTE the jekyll `where_exp` filter does not support `and` in 3.8.5
  - support has been added in 4.0.0, but cannot upgrade due to `github-pages` gem dependecny
  - see jekyll/jekyll#6998
  - see github/pages-gem#651 (comment)
@bguiz
Copy link
Contributor Author

bguiz commented Dec 18, 2019

@bguiz I'm not sure if it is what you tried to fix, but check the "{title}":

@alepc253 Thanks for this. It was caused by redirect pages, and I have pushed more commits which fix that now.

dulcedu
dulcedu approved these changes Dec 20, 2019
View reviewed changes
Copy link
Contributor

@dulcedu dulcedu left a comment

Choose a reason for hiding this comment

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

ok :)

alepc253
alepc253 approved these changes Dec 20, 2019
View reviewed changes
Copy link
Contributor

@alepc253 alepc253 left a comment

Choose a reason for hiding this comment

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

LGTM

@alebanzas alebanzas merged commit a2e3948 into master Dec 20, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/search branch December 20, 2019 13:55
@bguiz bguiz mentioned this pull request Dec 30, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dulcedu dulcedu dulcedu approved these changes

@alepc253 alepc253 alepc253 approved these changes

@vojtechsimetka vojtechsimetka Awaiting requested review from vojtechsimetka

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bguiz @alepc253 @dulcedu @alebanzas