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

Fix minor search summary issues #10548

Merged
merged 3 commits into from Jul 12, 2022
Merged

Conversation

shiftinv
Copy link
Contributor

@shiftinv shiftinv commented Jun 13, 2022

Subject: Fix minor search summary issues

Feature or Bugfix

  • Bugfix

Purpose

I believe all of these are mainly cosmetic minor regressions from #10028.
See 5.x vs this branch for comparison (with rtd's server-side search disabled as it's not an issue when that's used).

Details

  • Summary element type should be <p> instead of <div>, fixes styling (originally changed in 339cab7)
  • The lowercase text should only be used for context matching, not for the final output
  • Removal of header links was broken

Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

Are these severe enough regressions to go in 5.0.2?

A

@@ -158,7 +158,7 @@ const Search = {
const htmlElement = document
.createRange()
.createContextualFragment(htmlString);
_removeChildren(htmlElement.querySelectorAll(".headerlink"));
htmlElement.querySelectorAll(".headerlink").forEach((el) => el.parentNode.removeChild(el));
Copy link
Member

Choose a reason for hiding this comment

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

What's the functional change here?

Copy link
Contributor Author

@shiftinv shiftinv Jun 13, 2022

Choose a reason for hiding this comment

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

This previously used jQuery's htmlElement.find('.headerlink').remove(), which would remove the .headerlinks themselves.
As far as I can tell, _removeChildren is meant to be a replacement for .empty() - it only takes a single element instead of a NodeList as returned by querySelectorAll, and would only remove child elements instead of the element itself; as it stands right now I'm fairly sure this particular call is effectively a no-op.

The functional change here is just getting rid of the glyphs in the output as intended.

@AA-Turner AA-Turner added this to the 5.1.0 milestone Jun 13, 2022
@shiftinv
Copy link
Contributor Author

Are these severe enough regressions to go in 5.0.2?

The div/p change might be, the original switch to p was marked as a breaking change in 4.0 so this regression might be considered moderate, not sure how much relies on it.
Let me know if I should split the changes into separate PRs, these initially just seemed small enough to go into one PR.

Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

LGTM!

@tk0miya tk0miya mentioned this pull request Jun 26, 2022
@AA-Turner AA-Turner merged commit ede71fd into sphinx-doc:5.x Jul 12, 2022
AA-Turner added a commit that referenced this pull request Jul 12, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants