Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a sponsor-box UI block and corresponding CSS: new color variables, light/dark visibility toggles, left-accent border and image alignment rules; and inserts sponsor logos/links into the site's index content with light/dark variants. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.rst`:
- Around line 31-33: The call-to-action contains a duplicated word; locate the
sentence containing the phrase "Reach out <mailto:stefanv@berkeley.edu>`__ out
if you would" and remove the extra "out" so it reads "Reach out
<mailto:stefanv@berkeley.edu>`__ if you would" (or rephrase to "Reach out to
stefanv@berkeley.edu if you would" for clarity).
- Line 29: The current substitutions using |czi_logo| |czi_logo_dark|
|nominal_logo| |nominal_logo_dark| |bids_logo| |bids_logo_dark| leave empty
focusable anchors because Sphinx applies :class: only to the <img>; replace each
sponsor's six substitution usages with a single accessible linked block that
moves the visibility classes onto the anchor and uses a <picture> with media
queries inside the <a> so the <a> is hidden when the variant should be hidden
(i.e., make the anchor carry .only-light/.only-dark instead of the <img>),
ensure the <a> has the target href and descriptive aria-label/text, and repeat
this pattern for each sponsor (CZI, Nominal, BIDS) so no empty anchors remain in
the DOM.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29640cec-a191-421f-a4df-8b6efaa97395
⛔ Files ignored due to path filters (6)
_static/bids_logo.svgis excluded by!**/*.svg_static/bids_logo_dark.svgis excluded by!**/*.svg_static/czi_logo.svgis excluded by!**/*.svg_static/czi_logo_dark.svgis excluded by!**/*.svg_static/nominal_logo.svgis excluded by!**/*.svg_static/nominal_logo_dark.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
_static/custom.cssindex.rst
|
|
||
| The scikit-image team is hard at work at bringing you **scikit-image v2**, a major overhaul with a cleaner and more intuitive API. We would like to thank our project sponsors: | ||
|
|
||
| |czi_logo| |czi_logo_dark| |nominal_logo| |nominal_logo_dark| |bids_logo| |bids_logo_dark| |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the index.rst file
find . -name "index.rst" -type f | head -20Repository: scikit-image/skimage-web
Length of output: 79
🏁 Script executed:
# Also check for the custom.css mentioned in the comment
find . -name "custom.css" -type fRepository: scikit-image/skimage-web
Length of output: 88
🏁 Script executed:
# List the directory structure to understand the layout
git ls-files | grep -E "\.(rst|css)$" | head -30Repository: scikit-image/skimage-web
Length of output: 171
🏁 Script executed:
# Examine index.rst to see the sponsor logo markup
cat -n index.rst | head -70 | tail -50Repository: scikit-image/skimage-web
Length of output: 2032
🏁 Script executed:
# Examine custom.css to see the visibility toggle CSS
cat -n _static/custom.css | head -90 | tail -20Repository: scikit-image/skimage-web
Length of output: 657
🌐 Web query:
Sphinx Docutils image directive :target: :class: HTML rendering behavior
💡 Result:
In Sphinx (HTML builders), the reStructuredText/Docutils .. image:: options behave like this:
-
:target:makes the image “clickable” by nesting the<image>node inside a<reference>node (“ (only with option target)”). In HTML this becomes an<a ...>wrapping the<img ...>. [1]
In Sphinx’s HTML5 translator, when a<reference>contains only an image, Sphinx adds the CSS classimage-referenceto the<a>element (in addition toreferenceandinternal/external). [2] -
:class:on.. image::sets the Docutilsclassesattribute on the doctree element generated by the directive—forimage, that’s the<image>node—so in HTML it ends up on the<img>element’sclass="..."(not on the wrapping<a>created by:target:). [1][3] -
Interaction with Sphinx’s auto-linking of scaled images: if
html_scaled_image_link = True(default), Sphinx may also wrap resized images in a link to the original image, but it will not override a link provided by:target:; you can disable the auto-linking per image by adding theno-scaled-linkclass. [4]
Typical HTML shape with both options (schematic):
<a class="reference internal image-reference" href="...">
<img class="your-class" src="..." alt="...">
</a>If you need a class on the <a>: :class: won’t do it; you’d need a different construct (e.g., explicit reST hyperlink wrapping, a custom directive/role, or raw HTML), because the class option targets the image node, not the generated reference node. [1][2][3]
Sources: Docutils image directive (target nests image in reference; class sets classes on generated element). [1][3] Sphinx HTML translator adding image-reference on image-only links. [2] Sphinx html_scaled_image_link behavior and its interaction with :target: / no-scaled-link. [4]
Hide the linked sponsor variant, not just the image.
When Sphinx renders .. image:: with :target:, it wraps the <img> in an <a> element. The :class: option applies only to the <img>, so display: none on .only-light hides the image but leaves the empty <a> in the DOM—creating invisible, focusable links and weak screen-reader output. Replace the six substitutions with a single accessible link per sponsor using <picture> media queries:
Pattern for one sponsor logo
- |czi_logo| |czi_logo_dark|
+ .. raw:: html
+
+ <a href="https://chanzuckerberg.com/eoss/">
+ <picture>
+ <source srcset="_static/czi_logo_dark.svg" media="(prefers-color-scheme: dark)">
+ <img src="_static/czi_logo.svg"
+ alt="Chan Zuckerberg Initiative EOSS"
+ height="40">
+ </picture>
+ </a>Repeat for the other two sponsors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.rst` at line 29, The current substitutions using |czi_logo|
|czi_logo_dark| |nominal_logo| |nominal_logo_dark| |bids_logo| |bids_logo_dark|
leave empty focusable anchors because Sphinx applies :class: only to the <img>;
replace each sponsor's six substitution usages with a single accessible linked
block that moves the visibility classes onto the anchor and uses a <picture>
with media queries inside the <a> so the <a> is hidden when the variant should
be hidden (i.e., make the anchor carry .only-light/.only-dark instead of the
<img>), ensure the <a> has the target href and descriptive aria-label/text, and
repeat this pattern for each sponsor (CZI, Nominal, BIDS) so no empty anchors
remain in the DOM.
jni
left a comment
jni
left a comment
There was a problem hiding this comment.
Great! I also think adding our individual (eg $5 monthly) sponsors would be a nice move.
|
Very cool! |
lagru
left a comment
lagru
left a comment
There was a problem hiding this comment.
Happy to add these.
I tried to look at the preview on CircleCI for the last commit, maybe I'm being dumb but I can't find the logos on there?
|
Thanks, Lars, I'll investigate what's hiding the box. It builds fine locally, so may be version of Sphinx or similar. |
|
Hah, @lagru, it's our ad blockers 👀 May have to rename those CSS classes! |
4 participants
No description provided.