feat(website): add docs structure and pages#22
Merged
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a Hugo-based documentation site: new docs content and data, Hugo templates/partials/shortcodes, styles, a Docs CTA in the header, and a Makefile Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🧹 Nitpick comments (1)
Makefile (1)
85-92: Optional: simplifysiterecipe to clear the linter warning.Line 85–92 is functionally fine, but checkmake flags target body length. A compact two-line recipe keeps behavior and removes warning noise.
♻️ Proposed refactor
site: - `@if` command -v hugo >/dev/null 2>&1; then \ - hugo server --source website; \ - else \ - echo "hugo is not installed."; \ - echo "Install Hugo and retry."; \ - exit 1; \ - fi + `@command` -v hugo >/dev/null 2>&1 || (echo "hugo is not installed."; echo "Install Hugo and retry."; exit 1) + `@hugo` server --source website🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 85 - 92, The Makefile 'site' target should be collapsed to a compact two-line recipe to address the checkmake warning: replace the multi-line if/else block in the site target with a short guard using 'command -v hugo >/dev/null 2>&1 || { echo "hugo is not installed."; echo "Install Hugo and retry."; exit 1; }' on one line followed by 'hugo server --source website' on the next line so behavior is unchanged but the recipe body is shorter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@website/content/docs/launch-options.md`:
- Around line 33-35: The examples using the "perspecta://open?dicomweb=..." URL
with user/password query parameters promote placing credentials in URLs; update
the document to remove or replace those credentialized examples with a
local/testing-only warning and a secure alternative (e.g., mention using
authentication headers or prompt-based login), and add an explicit cautionary
note near the shown examples and the similar block referenced (the later block
at lines 51-57) that embedding secrets in URLs is unsafe because they can be
logged or persisted; ensure the examples are scoped to "local/testing only" and
include the security warning text adjacent to the example strings.
In `@website/static/css/styles.css`:
- Around line 53-55: The .nav-toggle rule currently uses display: none which
removes the checkbox from the accessibility tree and prevents keyboard focus;
replace that with a visually-hidden-but-focusable pattern so the input remains
focusable and the paired <label> can toggle it via keyboard. Update the
.nav-toggle CSS (and the equivalent rules at the other block) to use an
accessible visually-hidden style (e.g., position: absolute; width: 1px; height:
1px; padding: 0; margin: -1px; overflow: hidden; clip: rect(0 0 0 0);
white-space: nowrap; border: 0;) instead of display: none, and ensure the label
selector (e.g., .nav-toggle + label) has visible cursor and focus styles so
keyboard users can see/focus it.
---
Nitpick comments:
In `@Makefile`:
- Around line 85-92: The Makefile 'site' target should be collapsed to a compact
two-line recipe to address the checkmake warning: replace the multi-line if/else
block in the site target with a short guard using 'command -v hugo >/dev/null
2>&1 || { echo "hugo is not installed."; echo "Install Hugo and retry."; exit 1;
}' on one line followed by 'hugo server --source website' on the next line so
behavior is unchanged but the recipe body is shorter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d13e771-eb0a-4663-87a1-410259e90777
📒 Files selected for processing (17)
Makefilewebsite/content/docs/_index.mdwebsite/content/docs/dicomweb.mdwebsite/content/docs/install/_index.mdwebsite/content/docs/install/linux.mdwebsite/content/docs/install/macos.mdwebsite/content/docs/install/windows.mdwebsite/content/docs/launch-options.mdwebsite/data/install_platforms.tomlwebsite/hugo.tomlwebsite/layouts/docs/list.htmlwebsite/layouts/docs/single.htmlwebsite/layouts/index.htmlwebsite/layouts/partials/docs-sidebar.htmlwebsite/layouts/partials/header.htmlwebsite/layouts/shortcodes/install-platform.htmlwebsite/static/css/styles.css
There was a problem hiding this comment.
🧹 Nitpick comments (1)
website/static/css/styles.css (1)
60-60: Replace deprecatedclipproperty withclip-path.The
clipproperty is deprecated per CSS specifications. Useclip-path: inset(50%)for modern browsers, keepingclipas a fallback for older browsers.♻️ Suggested fix
.nav-toggle { position: absolute; width: 1px; height: 1px; padding: 0; margin: -1px; overflow: hidden; clip: rect(0 0 0 0); + clip-path: inset(50%); white-space: nowrap; border: 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/static/css/styles.css` at line 60, Replace the deprecated CSS property usage of clip with a modern clip-path fallback pattern: keep the existing clip declaration as a backward-compatible fallback, then add clip-path: inset(50%); (and optionally clip-path: inset(50%) round 0;) to the same selector so modern browsers use clip-path while older browsers still honor clip; update the styles.css rule that currently contains clip: rect(0 0 0 0); to include the new clip-path declaration and ensure visual equivalence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@website/static/css/styles.css`:
- Line 60: Replace the deprecated CSS property usage of clip with a modern
clip-path fallback pattern: keep the existing clip declaration as a
backward-compatible fallback, then add clip-path: inset(50%); (and optionally
clip-path: inset(50%) round 0;) to the same selector so modern browsers use
clip-path while older browsers still honor clip; update the styles.css rule that
currently contains clip: rect(0 0 0 0); to include the new clip-path declaration
and ensure visual equivalence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77acaf73-7142-49f8-8822-d6061ce56485
📒 Files selected for processing (3)
Makefilewebsite/content/docs/launch-options.mdwebsite/static/css/styles.css
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 `@website/static/css/styles.css`:
- Around line 53-67: The .nav-toggle element is currently visually-hidden but
still focusable on desktop; update styles so .nav-toggle is non-focusable/hidden
by default (e.g., add display: none to .nav-toggle) and move the
visually-hidden-but-focusable rules
(padding/margin/overflow/clip/clip-path/white-space/border) into the mobile
media query that shows .nav-toggle-button (the same block where
.nav-toggle-button is set to display:block) so the hidden-but-focusable pattern
only applies on mobile; apply the same change for the other instance referenced
(the rules around .nav-toggle at lines ~607-610).
- Line 60: Remove the deprecated CSS property by deleting the `clip: rect(0 0 0
0);` declaration in the ruleset where `clip-path: inset(50%)` is already used
(the block containing the `clip` declaration); keep the modern `clip-path` and
any other accessibility-related styles intact so the visual hiding behavior
remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 479a18a1-220a-4067-beac-d7f4d82ba601
📒 Files selected for processing (1)
website/static/css/styles.css
Comment on lines
+53
to
+67
| .nav-toggle { | ||
| position: absolute; | ||
| width: 1px; | ||
| height: 1px; | ||
| padding: 0; | ||
| margin: -1px; | ||
| overflow: hidden; | ||
| clip: rect(0 0 0 0); | ||
| clip-path: inset(50%); | ||
| white-space: nowrap; | ||
| border: 0; | ||
| } | ||
|
|
||
| .nav-toggle-button { | ||
| display: none; |
There was a problem hiding this comment.
Avoid an invisible keyboard tab stop on desktop navigation.
At Line 53-64, .nav-toggle is focusable at all breakpoints, but its visible control is hidden by default at Line 67. That leaves desktop keyboard users with an off-screen focus target. Keep it hidden on desktop and apply the visually-hidden-focusable pattern only in the mobile media block.
Suggested fix
.nav-toggle {
- position: absolute;
- width: 1px;
- height: 1px;
- padding: 0;
- margin: -1px;
- overflow: hidden;
- clip: rect(0 0 0 0);
- clip-path: inset(50%);
- white-space: nowrap;
- border: 0;
+ display: none;
}
`@media` (max-width: 860px) {
+ .nav-toggle {
+ position: absolute;
+ width: 1px;
+ height: 1px;
+ padding: 0;
+ margin: -1px;
+ overflow: hidden;
+ clip: rect(0 0 0 0);
+ clip-path: inset(50%);
+ white-space: nowrap;
+ border: 0;
+ }
+
.nav-toggle-button {
display: inline-flex;
}Also applies to: 607-610
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 60-60: Unexpected deprecated property "clip" (property-no-deprecated)
(property-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@website/static/css/styles.css` around lines 53 - 67, The .nav-toggle element
is currently visually-hidden but still focusable on desktop; update styles so
.nav-toggle is non-focusable/hidden by default (e.g., add display: none to
.nav-toggle) and move the visually-hidden-but-focusable rules
(padding/margin/overflow/clip/clip-path/white-space/border) into the mobile
media query that shows .nav-toggle-button (the same block where
.nav-toggle-button is set to display:block) so the hidden-but-focusable pattern
only applies on mobile; apply the same change for the other instance referenced
(the rules around .nav-toggle at lines ~607-610).
timcogan
force-pushed
the
feat/website-docs
branch
from
b5fdd14 to
d8f90de
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
website/static/css/styles.css (1)
180-184: Add:focus-visibleparity for newly added hover affordances.Several interactive elements now have hover-specific styling only. Mirroring those styles for keyboard focus will make navigation feedback consistent.
Proposed CSS adjustment
-.button:hover { +.button:hover, +.button:focus-visible { background: rgba(255, 255, 255, 0.04); border-color: `#3b4352`; color: var(--fg); } -.button.primary:hover { +.button.primary:hover, +.button.primary:focus-visible { background: `#e7edf7`; border-color: `#e7edf7`; box-shadow: 0 0 0 1px rgba(231, 237, 247, 0.35); } -.docs-nav-link:hover { +.docs-nav-link:hover, +.docs-nav-link:focus-visible { border-color: var(--line); color: var(--fg); } -.docs-nav-sublink:hover { +.docs-nav-sublink:hover, +.docs-nav-sublink:focus-visible { border-color: var(--line); color: var(--fg); } -.doc-card:hover { +.doc-card:hover, +.doc-card:focus-visible { border-color: `#4f617f`; background: rgba(79, 97, 127, 0.08); } -.links > a:hover { +.links > a:hover, +.links > a:focus-visible { border-color: var(--line); background: rgba(255, 255, 255, 0.03); }Also applies to: 192-196, 467-470, 496-499, 557-560, 636-639
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/static/css/styles.css` around lines 180 - 184, The hover-only styles for interactive elements (e.g., the .button:hover rule) need equivalent :focus-visible rules so keyboard users get the same affordances; update each selector that defines hover-only styling (such as .button:hover and the other hover selectors noted at ranges 192-196, 467-470, 496-499, 557-560, 636-639) by adding matching :focus-visible selectors that mirror background, border-color, and color changes (use :focus-visible rather than :focus to avoid styling mouse-focus), ensuring visual parity and consistent keyboard navigation feedback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@website/static/css/styles.css`:
- Around line 180-184: The hover-only styles for interactive elements (e.g., the
.button:hover rule) need equivalent :focus-visible rules so keyboard users get
the same affordances; update each selector that defines hover-only styling (such
as .button:hover and the other hover selectors noted at ranges 192-196, 467-470,
496-499, 557-560, 636-639) by adding matching :focus-visible selectors that
mirror background, border-color, and color changes (use :focus-visible rather
than :focus to avoid styling mouse-focus), ensuring visual parity and consistent
keyboard navigation feedback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c79eab8-b9d3-4359-a08a-7d710fadf838
📒 Files selected for processing (1)
website/static/css/styles.css
There was a problem hiding this comment.
🧹 Nitpick comments (1)
website/static/css/styles.css (1)
584-591: Consider increasing padding-bottom to accommodate focus outlines.The
padding-bottom: 2pxon.doc-chip-rowmay clip focus outlines when the row scrolls horizontally. Button focus outlines typically need more clearance.Suggested fix
.doc-chip-row { margin-top: 18px; display: flex; flex-wrap: nowrap; gap: 10px; overflow-x: auto; - padding-bottom: 2px; + padding-bottom: 4px; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/static/css/styles.css` around lines 584 - 591, The .doc-chip-row container uses padding-bottom: 2px which can clip button focus outlines when scrolled; update the .doc-chip-row rule to increase padding-bottom (e.g., to 6px or more) or add a larger bottom padding value to ensure focus outlines are not clipped (alternatively set a matching outline-offset on focused buttons), and verify visually that focus rings for elements inside .doc-chip-row are fully visible when the row is scrolled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@website/static/css/styles.css`:
- Around line 584-591: The .doc-chip-row container uses padding-bottom: 2px
which can clip button focus outlines when scrolled; update the .doc-chip-row
rule to increase padding-bottom (e.g., to 6px or more) or add a larger bottom
padding value to ensure focus outlines are not clipped (alternatively set a
matching outline-offset on focused buttons), and verify visually that focus
rings for elements inside .doc-chip-row are fully visible when the row is
scrolled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e603f99f-962c-4cb3-8bac-92b74873f700
📒 Files selected for processing (1)
website/static/css/styles.css
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Style