redirect frontend GUI #269
Conversation
WalkthroughThis pull request introduces modifications to the CSS styles and TypeScript functionality in the kiosk application, focusing on handling redirects. Key changes include the addition of a new CSS file for redirects, updates to existing styles for the Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/src/ts/menu.ts (1)
75-88: Consider adding keyboard shortcuts for closing the overlay.The keyboard navigation is well-implemented with circular navigation and scroll prevention. Consider adding the Escape key handler to close the overlay.
function redirectKeyHandler(e: KeyboardEvent) { if (redirects) { + if (e.key === "Escape") { + hideRedirectsOverlay(); + return; + } if (e.key === "ArrowDown") { e.preventDefault(); // Prevent page scrolling currentRedirectIndex = (currentRedirectIndex + 1) % redirects.length;frontend/src/css/redirects.css (3)
3-5: Consider touch interactions for text selection.The universal selector enabling text selection might interfere with smooth touch interactions on mobile devices. Consider using a more specific selector or adding touch-friendly styles.
- * { - user-select: text; - } + a { + user-select: text; + -webkit-tap-highlight-color: transparent; + }
40-40: Useoverflow-y: autofor better UX.Using
overflow: scrollforces scrollbars to appear even when content fits. Useoverflow-y: autoto show scrollbars only when needed.- overflow: scroll; + overflow-y: auto;
45-51: Consolidate duplicate padding rules.The same padding-top value is duplicated for first and last child. Consider using a single selector.
- .redirects a:first-child { - padding-top: 1rem; - } - - .redirects a:last-child { - padding-top: 1rem; - } + .redirects a:first-child, + .redirects a:last-child { + padding-top: 1rem; + }frontend/public/assets/css/kiosk.css (1)
1320-1394: Fix typo and enhance scroll behaviour.The redirects overlay implementation is well-structured and consistent with the existing more-info overlay. However, there are a few improvements to consider:
- There's a typo in the rgba colour value on line 1379:
rgba(255, 2555, 255, 0.2)should bergba(255, 255, 255, 0.2).- Consider using
overflow: autoinstead ofoverflow: scrollto show scrollbars only when needed.Apply this diff to fix the issues:
- overflow: scroll; + overflow: auto;- border-bottom: 1px solid rgba(255, 2555, 255, 0.2); + border-bottom: 1px solid rgba(255, 255, 255, 0.2);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
frontend/public/assets/css/kiosk.css(5 hunks)frontend/src/css/clock-weather-container.css(1 hunks)frontend/src/css/image.css(1 hunks)frontend/src/css/kiosk.css(2 hunks)frontend/src/css/menu.css(1 hunks)frontend/src/css/redirects.css(1 hunks)frontend/src/ts/kiosk.ts(3 hunks)frontend/src/ts/menu.ts(5 hunks)internal/templates/partials/menu.templ(3 hunks)internal/templates/partials/redirects.templ(1 hunks)internal/templates/views/views_home.templ(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/css/kiosk.css
🧰 Additional context used
📓 Path-based instructions (1)
frontend/public/assets/css/kiosk.css (1)
Pattern frontend/public/**: exclude
🔇 Additional comments (16)
internal/templates/partials/redirects.templ (1)
5-13: LGTM! Well-structured template with proper URL sanitisation.The template follows security best practices by using
templ.SafeURLto prevent XSS attacks.internal/templates/views/views_home.templ (1)
54-54: LGTM! Proper parameter passing to the menu template.The update correctly passes the redirects data to the menu template.
frontend/src/ts/menu.ts (3)
12-18: LGTM! Well-structured variable declarations.Clear type declarations and meaningful variable names.
95-104: LGTM! Proper event listener management.The overlay management functions properly handle event listener cleanup.
Also applies to: 109-116
140-142: LGTM! Safe initialization with null check.The initialization safely handles the case when redirects container is not present.
internal/templates/partials/menu.templ (3)
5-5: LGTM! Clear parameter declaration.The template signature is well-defined with proper types.
26-28: LGTM! Proper null check for redirects.The conditional rendering safely handles empty redirects.
62-67: LGTM! Semantic SVG implementation.The links button follows the established pattern and uses proper SVG attributes.
frontend/src/ts/kiosk.ts (3)
19-19: LGTM! Import statement follows existing pattern.The addition of
toggleRedirectsOverlayto the imports is consistent with how other overlay-related functions are imported.
101-101: LGTM! Button declaration follows type safety practices.The
linksButtonconstant is properly typed withHTMLElement | null, consistent with other button declarations in the file.
237-239: LGTM! Event listener implementation is consistent.The event listener for the links button follows the same pattern as other overlay toggles, maintaining consistency in the codebase.
frontend/src/css/clock-weather-container.css (1)
1-2: LGTM! Consistent styling for overlay states.The addition of
.polling-paused.redirects-openselector maintains visual consistency by reusing the same opacity rules applied to the more-info overlay.frontend/src/css/menu.css (1)
115-133: LGTM! Active states follow existing patterns.The implementation of active states for the more-info and redirects buttons is consistent with other menu items and uses modern CSS nesting appropriately.
frontend/src/css/image.css (1)
2-3: LGTM! Consistent implementation of the redirects overlay.The new selector
.polling-paused.redirects-openfollows the same pattern as the existing.polling-paused.more-infoselector, maintaining consistency in the UI behaviour.frontend/public/assets/css/kiosk.css (2)
1239-1244: LGTM! Consistent styling of the redirects button.The styles for
.polling-paused.redirects-open .navigation--linksfollow the same pattern as the more-info button, maintaining consistency in the UI.
752-752: LGTM! Consistent opacity handling for the clock-weather container.The new selector
.polling-paused.redirects-openis correctly grouped with.polling-paused.more-info, maintaining consistency in the UI behaviour.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/src/ts/menu.ts (3)
75-91: Consider enhancing keyboard navigation accessibility.The keyboard handler should include:
- 'Escape' key support for closing the overlay
- 'Home' and 'End' keys for quick navigation
- Focus restoration when closing the overlay
Here's a suggested enhancement:
function redirectKeyHandler(e: KeyboardEvent) { if (!redirects) return; switch (e.code) { + case "Escape": + hideRedirectsOverlay(); + break; + case "Home": + e.preventDefault(); + currentRedirectIndex = 0; + redirects[currentRedirectIndex].focus(); + break; + case "End": + e.preventDefault(); + currentRedirectIndex = redirects.length - 1; + redirects[currentRedirectIndex].focus(); + break; case "ArrowDown": e.preventDefault(); // Prevent page scrolling currentRedirectIndex = (currentRedirectIndex + 1) % redirects.length;
98-107: Improve focus management in overlay functions.Consider storing and restoring focus when showing/hiding the overlay to enhance accessibility.
Here's a suggested enhancement:
+let previouslyFocusedElement: HTMLElement | null = null; + function showRedirectsOverlay(): void { if (!document.body) return; if (!document.body.classList.contains("polling-paused")) return; + previouslyFocusedElement = document.activeElement as HTMLElement; document.addEventListener("keydown", redirectKeyHandler); hideImageOverlay(); document.body.classList.add("redirects-open"); linkOverlayVisible = true; + if (redirects?.length) { + currentRedirectIndex = 0; + redirects[0].focus(); + } } function hideRedirectsOverlay(): void { if (!document.body) return; document.body.classList.remove("redirects-open"); document.removeEventListener("keydown", redirectKeyHandler); linkOverlayVisible = false; + currentRedirectIndex = -1; + previouslyFocusedElement?.focus(); + previouslyFocusedElement = null; }Also applies to: 112-119
142-145: Consider adding error handling for redirects initialization.While the null check is good, consider logging a warning when redirects are expected but not found, to help with debugging.
if (redirectsContainer) { redirects = redirectsContainer.querySelectorAll("a"); + if (!redirects.length) { + console.warn("No redirect links found in redirects container"); + } }frontend/public/assets/css/kiosk.css (1)
1321-1412: Consider using a more maintainable approach for scroll shadows.While the implementation is solid, the scroll shadow implementation could be simplified to improve maintainability.
Consider using a CSS-only solution with sticky pseudo-elements:
-.polling-paused.redirects-open #redirects-container .redirects .redirects--shadow::before, -.polling-paused.redirects-open #redirects-container .redirects .redirects--shadow::after { +.polling-paused.redirects-open #redirects-container .redirects::before, +.polling-paused.redirects-open #redirects-container .redirects::after { content: ""; - position: absolute; + position: sticky; left: 0; right: 0; height: 20%; pointer-events: none; background-image: radial-gradient(60% 50% at 50% 0%, var(--fade-gradient)); z-index: var(--z-below); opacity: 0.6; }This approach:
- Eliminates the need for an extra wrapper div
- Uses sticky positioning for better scroll behaviour
- Maintains the same visual effect with less markup
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/public/assets/css/kiosk.css(6 hunks)frontend/src/css/redirects.css(1 hunks)frontend/src/css/vars.css(1 hunks)frontend/src/ts/menu.ts(5 hunks)internal/templates/partials/redirects.templ(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/templates/partials/redirects.templ
- frontend/src/css/redirects.css
🧰 Additional context used
📓 Path-based instructions (1)
frontend/public/assets/css/kiosk.css (1)
Pattern frontend/public/**: exclude
🔇 Additional comments (6)
frontend/src/ts/menu.ts (2)
12-18: Well-structured variable declarations!The new variables are properly typed and initialised, following TypeScript best practices. The use of
-1forcurrentRedirectIndexis a good choice for representing an unselected state.
46-54: Good handling of overlay mutual exclusivity!The addition of
hideRedirectsOverlay()ensures that only one overlay is visible at a time, maintaining a clean user interface.frontend/src/css/vars.css (1)
9-9: LGTM! The z-index hierarchy is well-structured.The new
--z-belowvariable with value -1 follows the existing naming convention and maintains a logical progression of z-index values.frontend/public/assets/css/kiosk.css (3)
7-7: LGTM! Well-structured z-index variable.The new z-index variable is appropriately named and follows the existing pattern.
375-375: LGTM! Consistent opacity handling.The opacity adjustments for the polling-paused state with redirects open are consistent across components.
Also applies to: 753-753
1234-1245: LGTM! Well-structured navigation styles.The active states for navigation items follow consistent patterns and use the established colour variables.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/ts/kiosk.ts (2)
19-19: Typo in the import name 'toggleRedirectsOverlay'The import name and type property follow different naming conventions - 'Redirects' vs 'redirects'. Consider standardising the casing across the codebase.
Also applies to: 63-63
243-248: Add JSDoc for the new keyboard shortcutThe 'R' key shortcut lacks documentation unlike other keyboard shortcuts. Consider updating the event listener's JSDoc to include the new shortcut.
/** * Add event listeners to Kiosk elements * @description Configures interactive behavior by setting up: * - Menu click handlers for polling control - * - Keyboard shortcuts (space and 'i' keys) + * - Keyboard shortcuts (space, 'i', and 'r' keys) * - Fullscreen toggle functionality * - Image overlay controls * - HTMX error handling for offline states * - Server connectivity monitoring */Also applies to: 259-260
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/ts/kiosk.ts(6 hunks)internal/templates/partials/menu.templ(3 hunks)internal/templates/views/views_home.templ(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/templates/views/views_home.templ
🔇 Additional comments (4)
internal/templates/partials/menu.templ (3)
3-5: LGTM: Clean signature update for redirects supportThe function signature has been properly updated to accommodate the new redirects functionality, with appropriate type import.
62-69: LGTM: Well-structured navigation item for redirectsThe new navigation item follows the established pattern and uses appropriate iconography. The conditional rendering ensures consistency with the redirects availability check.
26-28: Verify the Redirects template implementationPlease ensure that the
Redirectstemplate is properly implemented and handles the redirects data appropriately.✅ Verification successful
Redirects template implementation verified
The Redirects template is properly implemented in
internal/templates/partials/redirects.templ. It correctly handles the redirects data structure and implements appropriate URL safety measures.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and implementation of the Redirects template # Search for the Redirects template definition echo "Searching for Redirects template definition..." ast-grep --pattern 'templ Redirects($_) { $$$ }' # Search for any files that might contain the Redirects template echo "Searching for files containing Redirects template..." rg -l "templ Redirects"Length of output: 339
Script:
#!/bin/bash # Examine the redirects template implementation echo "Content of redirects.templ:" cat internal/templates/partials/redirects.templLength of output: 480
frontend/src/ts/kiosk.ts (1)
102-102: Consider adding null check for linksButtonThe linksButton element is used in event listeners but lacks defensive null checks similar to other button elements in the codebase.
Run this script to check for consistent null handling across button elements:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/ts/kiosk.ts (1)
63-63: Add JSDoc comment for the new property.The
showRedirectsproperty lacks documentation. Consider adding a JSDoc comment to maintain consistency with other properties in the interface.* @property showMoreInfo - Show the more info image overlay + * @property showRedirects - Whether to display the redirects overlay */ type KioskData = {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/ts/kiosk.ts(6 hunks)
🔇 Additional comments (6)
frontend/src/ts/kiosk.ts (6)
19-19: LGTM! Import statement follows established patterns.The addition of
toggleRedirectsOverlayto the menu imports maintains consistency with related functionality.
102-102: LGTM! DOM element reference follows established patterns.The
linksButtonconstant is well-placed and properly typed, maintaining consistency with other button references.
180-201: LGTM! Well-structured helper function that reduces code duplication.The
handleOverlayTogglefunction effectively abstracts common overlay toggle logic with proper TypeScript typing and comprehensive documentation.
203-219: LGTM! Consistent implementation of keyboard shortcut handlers.Both
handleInfoKeyPressandhandleRedirectsKeyPressfunctions are well-documented and effectively utilise the shared helper function.
248-253: LGTM! Robust keyboard shortcut implementation.The 'r' key handler includes proper feature flag checking and browser shortcut conflict prevention.
264-266: LGTM! Event listener follows established patterns.The links overlay click handler maintains consistency with other overlay button handlers.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/src/ts/menu.ts (1)
160-162: Consider error handling for missing redirects containerThe current implementation silently handles a missing redirects container. Consider logging a warning or throwing an error if the container is required for functionality.
if (redirectsContainer) { redirects = redirectsContainer.querySelectorAll("a"); + } else { + console.warn("Redirects container not found. Redirect functionality will be disabled."); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/public/assets/css/kiosk.css(6 hunks)frontend/src/css/redirects.css(1 hunks)frontend/src/ts/kiosk.ts(8 hunks)frontend/src/ts/menu.ts(5 hunks)internal/templates/partials/redirects.templ(1 hunks)internal/templates/views/views_home.templ(2 hunks)taskfile.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- taskfile.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/templates/partials/redirects.templ
- internal/templates/views/views_home.templ
- frontend/src/ts/kiosk.ts
- frontend/src/css/redirects.css
- frontend/public/assets/css/kiosk.css
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/src/ts/menu.ts
[error] 21-21: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 22-22: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 151-151: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
[error] 152-152: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (3)
frontend/src/ts/menu.ts (3)
79-105: Well-implemented keyboard navigationThe keyboard handler is well-structured with:
- Proper event prevention for scrolling
- Circular navigation through links
- Thoughtful exclusion of browser shortcuts (Ctrl/Cmd + R)
107-140: Well-structured overlay managementThe implementation demonstrates good practices:
- Proper event listener cleanup
- Clear state management
- Mutual exclusivity between overlays
174-177: Well-organised exportsThe export list is properly organised and groups related functionality together.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/templates/partials/menu.templ (1)
26-28: Consider lazy loading of redirects menuThe redirects menu is always rendered in the DOM even when hidden. Consider lazy loading it when the toggle button is clicked to improve initial page load performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/templates/partials/menu.templ(3 hunks)internal/templates/partials/redirects.templ(1 hunks)internal/templates/views/views_home.templ(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/templates/views/views_home.templ
🔇 Additional comments (3)
internal/templates/partials/redirects.templ (2)
6-8: Well-structured container hierarchyGood use of semantic container structure with clear ID and class naming conventions.
13-17:⚠️ Potential issueValidate redirect.Name before using in URLs
The redirect.Name is directly used in URL construction without validation. This could potentially lead to open redirect vulnerabilities if the redirect configuration is compromised.
internal/templates/partials/menu.templ (1)
Line range hint
5-69: Well-structured navigation with proper event handlingGood implementation of:
- Event throttling to prevent rapid-fire requests
- Clear HTMX targeting and swap specifications
- Proper event cleanup handling
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/templates/partials/menu.templ (1)
Line range hint
8-31: Consider adding loading states for HTMX interactionsThe navigation area uses HTMX for dynamic content loading but lacks loading states.
Add HTMX indicators:
<nav id="navigation-interaction-area"> <div id="navigation-interaction-area--previous-image" hx-post="/image/previous" hx-target="#kiosk" hx-include=".kiosk-history--entry" + hx-indicator="#loading-indicator" hx-trigger="
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/src/ts/menu.ts(5 hunks)internal/templates/partials/menu.templ(3 hunks)internal/templates/partials/redirects.templ(1 hunks)internal/templates/views/views_home.templ(2 hunks)taskfile.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- taskfile.yml
- frontend/src/ts/menu.ts
- internal/templates/views/views_home.templ
🔇 Additional comments (1)
internal/templates/partials/menu.templ (1)
29-31: LGTM: Proper conditional rendering of redirectsThe conditional rendering of redirects is implemented correctly.
Summary by CodeRabbit
Release Notes
New Features
UI/UX Improvements
Styling Updates
The update provides a more interactive navigation experience with a new redirects feature and improved visual states.