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

HTML search: adjustments to type-dependent CSS classnames and defaults #12815

Merged

Conversation

jayaddison
Copy link
Contributor

@jayaddison jayaddison commented Aug 23, 2024

Feature or Bugfix

  • Bugfix
  • Refactoring

Purpose

  • This is a follow-up to Support type-dependent search result highlighting via CSS #12474 to resolve some quirks observed there.
  • Removes the default 'unicode white square' list item styling (in the built-in Sphinx13 theme) when no type-dependent information is found on search results (as is the case from RTD-based hosted search results).
  • Renames the type-dependent CSS classname indicator prefix from context- to kind- -- because we already use the term context elsewhere to refer to search summary snippets (and that is therefore a widely-deployed CSS classname).

Detail

  • Rename context- prefixes to kind-.
  • Remove the default Unicode 25A1 codepoint list-style-type for default search results (in the built-in sphinx13 theme).

Relates

cc @AA-Turner @timhoffm

@timhoffm
Copy link
Contributor

We also need to update the docs: https://www.sphinx-doc.org/en/master/development/html_themes/index.html#styling-with-css

In addition to the kind renaming, I suggest to add a note that this is only valid for the static builtin search.

Also, since sphinx13 is only used for the sphinx docs and they are deployed via rtd, I suggest to remove all the CSS added to it in #12474, because it does not have any effect.

@timhoffm
Copy link
Contributor

This is release-critical: The context naming has not been published yet. By renaming before a release, we prevent trouble with having the context naming in a public release.

@jayaddison
Copy link
Contributor Author

We also need to update the docs: https://www.sphinx-doc.org/en/master/development/html_themes/index.html#styling-with-css

In addition to the kind renaming, I suggest to add a note that this is only valid for the static builtin search.

Makes sense, yep - I'll add a note about that soon.

Also, since sphinx13 is only used for the sphinx docs and they are deployed via rtd, I suggest to remove all the CSS added to it in #12474, because it does not have any effect.

I'm not sure I agree with this; I fairly frequently (not always, by any means) build Sphinx's documentation locally during development and testing, and it's useful to have the functinality available there to check and inspect.

@timhoffm
Copy link
Contributor

I'm not sure I agree with this; I fairly frequently (not always, by any means) build Sphinx's documentation locally during development and testing, and it's useful to have the functinality available there to check and inspect.

Sure. In that case, add a comment that this is only for dev builds to avoid confusion for people looking into the code and trying to map it to released docs.

@jayaddison
Copy link
Contributor Author

@AA-Turner @picnixz could we fit this in before a v8.0.3 release? I had suggested a CSS naming convention in #12474 that overlaps with some already-in-use CSS classes; it'd be nice to avoid these appearing in a published release (an HTTP Referer-type situation).

@AA-Turner
Copy link
Member

Next release will be 8.1. Do we need to make a special 8.0.3 release?

A

@jayaddison
Copy link
Contributor Author

Next release will be 8.1. Do we need to make a special 8.0.3 release?

No, we do not need a patch/hotfix release for this; the context-... CSS prefix hasn't been included in any releases so far.

Provided that we merge this before the next release (8.1 in that case), then all is well.

@AA-Turner AA-Turner merged commit 92f024e into sphinx-doc:master Sep 18, 2024
23 checks passed
@jayaddison
Copy link
Contributor Author

Thank you @AA-Turner!

@jayaddison jayaddison deleted the pr-12474-followup/adjust-css-classname branch September 27, 2024 18:38
@AA-Turner AA-Turner added this to the 8.1.x milestone Oct 6, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 6, 2024
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.

3 participants