feat: add shared/button component #793
Conversation
WalkthroughAdds a new utilities surface (basePath, whenBasePathReady, getBasePath, loadStyleIntoShadow) and a utilities barrel export; introduces shared Button and Icon components with tests; replaces many inline SVGs and inline CSS with Icon usage and runtime stylesheet injection; adds theme tokens, button and flex CSS modules, and updated publish/watch styles; extends watch context with isFullscreen and toggleFullscreen; Rollup config now builds an utilities entry, copies assets, and reuses shared plugins; package.json exports "./utilities" and a prepare script; demo apps add a HANG_UI_ASSETS_PATH define and call setBasePath before importing UI elements. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 4
🧹 Nitpick comments (4)
js/hang-ui/src/Components/shared/styles/utils.ts (1)
10-16: Consider safer type handling for the root node.The type assertion on line 12 assumes
getRootNode()returns eitherShadowRootorDocument, butgetRootNode()can also return the element itself in some edge cases. While the"adoptedStyleSheets" in rootcheck provides a safety net, consider using a type guard for more explicit handling:export function useConstructableStyle(ref: HTMLElement | undefined, sheet: CSSStyleSheet): void { if (!ref) return; const root = ref.getRootNode(); if ("adoptedStyleSheets" in root && !root.adoptedStyleSheets.includes(sheet)) { root.adoptedStyleSheets = [...root.adoptedStyleSheets, sheet]; } }This avoids the type assertion entirely while maintaining the same behavior.
js/hang-ui/src/Components/shared/styles/flex.css (1)
1-5: Consider clarifying class semantics.The
.flexclass applies bothdisplay: flexand centering properties. This might be unexpected for developers who associate.flexwith justdisplay: flex(without automatic centering). Consider either:
- Renaming
.flexto.flex-centerfor clarity- Separating concerns:
.flex { display: flex; }and.center { justify-content: center; align-items: center; }The current implementation works but may lead to confusion if other flex layouts (flex-start, space-between, etc.) are needed later.
js/hang-ui/src/Components/shared/types/icons.ts (1)
3-3: Verify usage of theSVGIconstype.The
SVGIconstype is defined but doesn't appear to be used in the reviewed components. Components use eitherIcon(for single icons) orIconSet(for icon collections). Consider removing this type if it's unused or document its intended use case.Run the following script to check if
SVGIconsis used anywhere:#!/bin/bash # Description: Search for usage of SVGIcons type in the codebase # Search for SVGIcons type usage (excluding the definition itself) rg -n --type=ts --type=tsx 'SVGIcons' | rg -v 'export type SVGIcons'js/hang-ui/src/Components/watch/FullscreenButton.tsx (1)
54-56: Consider making the button title dynamic to match the icon state.The button title is currently static ("Fullscreen"), but the icon changes based on fullscreen state. For better accessibility and UX, the title should reflect the action: "Enter Fullscreen" or "Exit Fullscreen".
🔎 Proposed improvement for dynamic title
- <Button title="Fullscreen" onClick={onClick}> + <Button title={context.isFullscreen() ? "Exit Fullscreen" : "Enter Fullscreen"} onClick={onClick}> {context.isFullscreen() ? fullscreenIcons.exit() : fullscreenIcons.enter()} </Button>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockdeno.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
js/hang-ui/package.json(1 hunks)js/hang-ui/src/Components/publish/CameraSourceButton.tsx(2 hunks)js/hang-ui/src/Components/publish/FileSourceButton.tsx(2 hunks)js/hang-ui/src/Components/publish/MediaSourceSelector.tsx(2 hunks)js/hang-ui/src/Components/publish/MicrophoneSourceButton.tsx(2 hunks)js/hang-ui/src/Components/publish/NothingSourceButton.tsx(2 hunks)js/hang-ui/src/Components/publish/ScreenSourceButton.tsx(2 hunks)js/hang-ui/src/Components/publish/styles.css(0 hunks)js/hang-ui/src/Components/shared/__tests__/components/index.test.tsx(1 hunks)js/hang-ui/src/Components/shared/components/button/index.tsx(1 hunks)js/hang-ui/src/Components/shared/components/button/style.css(1 hunks)js/hang-ui/src/Components/shared/styles/flex.css(1 hunks)js/hang-ui/src/Components/shared/styles/utils.ts(1 hunks)js/hang-ui/src/Components/shared/styles/variables.css(1 hunks)js/hang-ui/src/Components/shared/types/icons.ts(1 hunks)js/hang-ui/src/Components/stats/__tests__/components/StatsItem.test.tsx(10 hunks)js/hang-ui/src/Components/stats/components/StatsPanel.tsx(4 hunks)js/hang-ui/src/Components/watch/FullscreenButton.tsx(1 hunks)js/hang-ui/src/Components/watch/PlayPauseButton.tsx(1 hunks)js/hang-ui/src/Components/watch/StatsButton.tsx(2 hunks)js/hang-ui/src/Components/watch/VolumeSlider.tsx(2 hunks)js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx(3 hunks)js/hang-ui/src/Components/watch/styles.css(0 hunks)
💤 Files with no reviewable changes (2)
- js/hang-ui/src/Components/watch/styles.css
- js/hang-ui/src/Components/publish/styles.css
🧰 Additional context used
🧬 Code graph analysis (12)
js/hang-ui/src/Components/watch/VolumeSlider.tsx (2)
js/hang-ui/src/Components/shared/types/icons.ts (1)
IconSet(6-6)js/hang-ui/src/Components/shared/components/button/index.tsx (1)
Button(44-68)
js/hang-ui/src/Components/watch/FullscreenButton.tsx (3)
js/hang-ui/src/Components/shared/types/icons.ts (1)
IconSet(6-6)js/hang-ui/src/Components/watch/useWatchUIContext.ts (1)
useWatchUIContext(4-12)js/hang-ui/src/Components/shared/components/button/index.tsx (1)
Button(44-68)
js/hang-ui/src/Components/publish/MediaSourceSelector.tsx (1)
js/hang-ui/src/Components/shared/components/button/index.tsx (1)
Button(44-68)
js/hang-ui/src/Components/watch/PlayPauseButton.tsx (3)
js/hang-ui/src/Components/shared/types/icons.ts (1)
IconSet(6-6)js/hang-ui/src/Components/watch/useWatchUIContext.ts (1)
useWatchUIContext(4-12)js/hang-ui/src/Components/shared/components/button/index.tsx (1)
Button(44-68)
js/hang-ui/src/Components/shared/components/button/index.tsx (2)
js/signals/src/dom.ts (1)
Element(65-65)js/hang-ui/src/Components/shared/styles/utils.ts (1)
useConstructableStyle(10-16)
js/hang-ui/src/Components/watch/StatsButton.tsx (2)
js/hang-ui/src/Components/shared/types/icons.ts (1)
Icon(7-7)js/hang-ui/src/Components/shared/components/button/index.tsx (1)
Button(44-68)
js/hang-ui/src/Components/publish/NothingSourceButton.tsx (2)
js/hang-ui/src/Components/shared/types/icons.ts (1)
Icon(7-7)js/hang-ui/src/Components/shared/components/button/index.tsx (1)
Button(44-68)
js/hang-ui/src/Components/publish/FileSourceButton.tsx (2)
js/hang-ui/src/Components/shared/types/icons.ts (1)
Icon(7-7)js/hang-ui/src/Components/shared/components/button/index.tsx (1)
Button(44-68)
js/hang-ui/src/Components/publish/MicrophoneSourceButton.tsx (2)
js/hang-ui/src/Components/shared/types/icons.ts (1)
Icon(7-7)js/hang-ui/src/Components/shared/components/button/index.tsx (1)
Button(44-68)
js/hang-ui/src/Components/publish/ScreenSourceButton.tsx (2)
js/hang-ui/src/Components/shared/types/icons.ts (1)
Icon(7-7)js/hang-ui/src/Components/shared/components/button/index.tsx (1)
Button(44-68)
js/hang-ui/src/Components/publish/CameraSourceButton.tsx (2)
js/hang-ui/src/Components/shared/types/icons.ts (1)
Icon(7-7)js/hang-ui/src/Components/shared/components/button/index.tsx (1)
Button(44-68)
js/hang-ui/src/Components/shared/types/icons.ts (1)
js/signals/src/dom.ts (1)
Element(65-65)
🔇 Additional comments (20)
js/hang-ui/src/Components/stats/__tests__/components/StatsItem.test.tsx (1)
41-41: LGTM! Consistent accessibility improvements across test fixtures.The addition of
aria-hidden="true"to all test SVGs correctly marks them as decorative elements, aligning with the accessibility improvements made throughout the PR. The removal of<title>elements is appropriate when usingaria-hidden.Also applies to: 61-61, 94-94, 114-114, 142-142, 169-169, 204-204, 234-234, 291-291, 315-315
js/hang-ui/src/Components/stats/components/StatsPanel.tsx (1)
25-25: LGTM! Consistent accessibility improvements for stats icons.Adding
aria-hidden="true"to all SVG icons correctly marks them as decorative elements, which is appropriate since the icon purpose is conveyed through the adjacent text labels (Network, Video, Audio, Buffer).Also applies to: 51-51, 73-73, 95-95
js/hang-ui/src/Components/shared/styles/variables.css (1)
9-16: Verify the spacing scale is intentional.The spacing variables use very large values (1rem to 32rem). Assuming a 16px base font size,
--spacing-32would be 512px, which is unusually large for UI spacing. Please confirm this scale is intentional and not meant to be in pixels (e.g.,--spacing-1: 1pxinstead of1rem).js/hang-ui/src/Components/watch/PlayPauseButton.tsx (2)
5-39: Well-structured icon set implementation.The
playbackIconsIconSet is properly typed and the SVGs are correctly marked witharia-hidden="true", allowing the Button component's title prop to provide accessibility context. The icons use consistent sizing and styling.Note: These icons reference
var(--color-white)on lines 12 and 28, which depends on the CSS variables being correctly defined. Ensure the typo invariables.cssis fixed (see comment on that file).
41-52: LGTM! Clean refactor to shared Button component.The component correctly uses the shared Button with dynamic title and icon based on playback state. The logic properly syncs the UI (icon/title) with the
isPlaying()state from context.js/hang-ui/package.json (1)
32-33: Package versions are current and secure.@solidjs/testing-library is at version 0.8.10, the latest version, and @testing-library/jest-dom is at version 6.9.1, the latest version. @testing-library/jest-dom 6.9.1 is the latest non-vulnerable version with healthy maintenance status. No updates required.
js/hang-ui/src/Components/publish/MediaSourceSelector.tsx (1)
2-2: LGTM! Clean migration to the shared Button component.The refactoring correctly replaces the native button with the shared Button component. The class update removes the now-unnecessary
publishButtonclass while retaining the specificmediaSourceVisibilityToggleclass for styling.Also applies to: 17-23
js/hang-ui/src/Components/publish/ScreenSourceButton.tsx (1)
1-22: LGTM! Excellent SVG icon implementation and accessibility.The screenIcon SVG is properly marked with
aria-hidden="true"(line 16), which correctly indicates it's decorative since the Button's title provides the accessible label. The refactoring maintains all functionality while improving code consistency.Also applies to: 34-40
js/hang-ui/src/Components/shared/__tests__/components/index.test.tsx (1)
1-60: Excellent test coverage for the Button component!The test suite comprehensively covers all Button functionality including:
- Default and custom props
- Event handling
- ARIA attributes (aria-label, aria-disabled)
- Disabled state and tabIndex
- Both string and JSX children
The tests follow best practices with proper use of
getByRoleand clear assertions.js/hang-ui/src/Components/watch/FullscreenButton.tsx (1)
5-44: LGTM! Well-structured icon set with proper accessibility.The
fullscreenIconsIconSet correctly defines both enter and exit states, with properaria-hidden="true"attributes on both SVGs (lines 17, 36). This follows the established pattern of decorative icons.js/hang-ui/src/Components/watch/StatsButton.tsx (1)
1-40: LGTM! Excellent refactoring with improved consistency and accessibility.The changes improve the component in multiple ways:
- SVG size increased to 24x24 for consistency with other icons
aria-hidden="true"properly marks the decorative icon (line 16)- Dynamic title provides clear feedback to users ("Show stats" vs "Hide stats")
- Explicit
Icontyping improves type safetyjs/hang-ui/src/Components/publish/MicrophoneSourceButton.tsx (1)
1-63: LGTM! Consistent implementation with proper device selection handling.The refactoring correctly:
- Replaces emoji with SVG icon marked
aria-hidden="true"(line 18)- Uses the shared Button component
- Maintains existing toggle behavior for mute/unmute
- Adds controlled source selection via
MediaSourceSourceSelectorwithselectedSourceandonSelectedprops- Properly updates audio device preferences in
onSourceSelectedusingpeek()to avoid unnecessary subscriptionsjs/hang-ui/src/Components/publish/NothingSourceButton.tsx (1)
5-21: No action needed. The CSS variable--color-redis defined injs/hang-ui/src/Components/shared/styles/variables.cssand is properly accessible to the component through the Button component's injected stylesheet.js/hang-ui/src/Components/watch/VolumeSlider.tsx (2)
6-90: LGTM! Well-structured icon system with proper accessibility.The volumeIcons definition and getVolumeIcon helper provide a clean, maintainable way to handle volume state visualization. All SVG icons correctly include
aria-hidden="true"for accessibility since the button itself has a descriptive title.
109-111: LGTM! Button integration is correct.The Button component is properly integrated with:
- Dynamic title for accessibility
- Correct icon rendering pattern (function call)
- Proper click handler wiring
js/hang-ui/src/Components/publish/FileSourceButton.tsx (2)
6-21: LGTM! Clean icon definition with proper accessibility.The fileIcon SVG is well-defined with
aria-hidden="true", following the established pattern for decorative icons.
46-52: LGTM! Button properly integrated with active state styling.The Button component correctly:
- Applies conditional active class
- Renders the fileIcon
- Wires the onClick handler
js/hang-ui/src/Components/shared/components/button/index.tsx (1)
1-68: LGTM! Well-designed accessible Button component.The implementation is solid with:
- Proper use of constructable stylesheets for style encapsulation
- Comprehensive accessibility support (aria-label, aria-disabled, disabled, tabIndex)
- Intelligent aria-label fallback chain
- Clean prop handling with sensible defaults
js/hang-ui/src/Components/publish/CameraSourceButton.tsx (2)
7-23: LGTM! Camera icon properly defined.The SVG icon follows the established pattern with proper
aria-hidden="true"for accessibility.
46-52: LGTM! Button integration is correct.The Button component is properly integrated with:
- Conditional active class based on camera state
- Correct icon rendering
- Proper onClick handler
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (1)
59-65: Excellent! Previous race condition is fixed.The critical issue from the previous review has been properly addressed by removing the synchronous
setIsFullscreencall. State updates now correctly happen via thefullscreenchangeevent listener.Optional: Consider error handling for edge cases.
Both
requestFullscreen()andexitFullscreen()return promises that can reject (e.g., browser restrictions, lack of user gesture). Adding.catch()handlers could improve UX by providing user feedback on failures, though in practice these failures are rare with modern browsers.🔎 Optional enhancement for error handling
const toggleFullscreen = () => { if (document.fullscreenElement) { - document.exitFullscreen(); + document.exitFullscreen().catch((err) => { + console.error("Failed to exit fullscreen:", err); + }); } else { - props.hangWatch.requestFullscreen(); + props.hangWatch.requestFullscreen().catch((err) => { + console.error("Failed to enter fullscreen:", err); + }); } };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
js/hang-ui/src/Components/shared/components/button/index.tsx(1 hunks)js/hang-ui/src/Components/shared/styles/variables.css(1 hunks)js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- js/hang-ui/src/Components/shared/components/button/index.tsx
🔇 Additional comments (5)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (4)
37-38: LGTM! Type definitions are correct.The additions to
WatchUIContextValuesproperly declare the new fullscreen API with appropriate types.
53-53: LGTM! Proper state initialization.Initializing from
document.fullscreenElementensures the signal reflects the actual fullscreen state on mount.
103-104: LGTM! Context value properly configured.The fullscreen API is correctly exposed through the context provider.
179-187: Perfect! Event listener correctly implements the recommended fix.The
fullscreenchangeevent handler ensuresisFullscreenstays synchronized with the actual document fullscreen state, completely resolving the race condition from the previous review. The cleanup function properly prevents memory leaks.js/hang-ui/src/Components/shared/styles/variables.css (1)
1-8: LGTM! Color variables and host scoping are correctly defined.The
:hostselector is appropriate for Shadow DOM/Web Components, and the color variables are well-defined. The typo fix from the previous review (--color-transparent) is confirmed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
js/hang-ui/src/Components/shared/styles/flex.css (1)
1-14: Optional: Simplify:is()usage and reduce redundancy.Two optional refactoring suggestions:
Unnecessary
:is()pseudo-class: Using:is()with a single selector adds no value. It's typically used to group multiple selectors. Consider using plain class selectors:.flex { /* ... */ } .flex--center { /* ... */ } .flex--align-center { /* ... */ }Redundant properties: Both
.flexand.flex--centerset identicaljustify-content: center; align-items: center;. When both are applied (as in the Button component), these properties are duplicated. Consider restructuring to avoid redundancy if both classes are frequently used together.js/hang-ui/src/Components/shared/components/button/index.tsx (1)
56-56: Optional: Redundant flex utility classes.The class string applies both
flexandflex--center, which results in duplicatejustify-content: center; align-items: center;declarations (both utilities set these properties). This redundancy stems from the flex utility design noted inflex.css.While not breaking functionality, consider simplifying once the flex utilities are restructured per the recommendations in
js/hang-ui/src/Components/shared/styles/flex.css.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
js/hang-ui/src/Components/publish/MediaSourceSelector.tsxjs/hang-ui/src/Components/publish/styles.cssjs/hang-ui/src/Components/shared/components/button/index.tsxjs/hang-ui/src/Components/shared/components/button/style.cssjs/hang-ui/src/Components/shared/styles/flex.cssjs/hang-ui/src/Components/shared/styles/variables.cssjs/hang-ui/src/Components/watch/PlayPauseButton.tsxjs/hang-ui/src/Components/watch/WatchControls.tsxjs/hang-ui/src/Components/watch/styles.css
✅ Files skipped from review due to trivial changes (1)
- js/hang-ui/src/Components/watch/WatchControls.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- js/hang-ui/src/Components/publish/MediaSourceSelector.tsx
- js/hang-ui/src/Components/watch/styles.css
- js/hang-ui/src/Components/shared/styles/variables.css
🧰 Additional context used
🧬 Code graph analysis (2)
js/hang-ui/src/Components/watch/PlayPauseButton.tsx (3)
js/hang-ui/src/Components/shared/types/icons.ts (1)
IconSet(6-6)js/hang-ui/src/Components/watch/useWatchUIContext.ts (1)
useWatchUIContext(4-12)js/hang-ui/src/Components/shared/components/button/index.tsx (1)
Button(44-68)
js/hang-ui/src/Components/shared/components/button/index.tsx (1)
js/hang-ui/src/Components/shared/styles/utils.ts (1)
useConstructableStyle(10-16)
🔇 Additional comments (5)
js/hang-ui/src/Components/publish/styles.css (1)
34-39: LGTM: Style overrides for media source selector buttons.The CSS correctly uses specificity to override the base Button styles, removing background, shadow, and border for media source selector buttons in both default and hover states.
js/hang-ui/src/Components/watch/PlayPauseButton.tsx (2)
5-39: LGTM: Well-structured icon definitions.The
playbackIconsIconSet is cleanly defined with proper accessibility attributes (aria-hidden="true") on the decorative SVGs. The icons use CSS variables for theming, which maintains consistency with the design system.
48-50: LGTM: Accessible button implementation.The Button correctly uses dynamic
titleattributes that change based on playback state, providing proper accessibility. The conditional icon rendering based oncontext.isPlaying()is clean and straightforward.js/hang-ui/src/Components/shared/components/button/index.tsx (2)
6-7: LGTM: Efficient constructable stylesheet setup.Creating the
CSSStyleSheetat module scope ensures it's shared across all Button instances, which is an efficient pattern for style encapsulation.
58-60: LGTM: Robust aria-label fallback logic.The fallback chain (
ariaLabel→title→ stringchildren) ensures proper accessibility labeling, with appropriate handling of non-string children that can't be used as labels.
pzanella
force-pushed
the
feat/button-icons
branch
from
5b70771 to
4f21ed1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
js/hang-ui/src/Components/watch/VolumeSlider.tsx (2)
6-78: Consider extracting common SVG attributes to reduce duplication.The four SVG icons share identical attributes (xmlns, dimensions, viewBox, fill, stroke properties, aria-hidden). While the current implementation is clear and functional, extracting these common attributes into a wrapper component or utility function would improve maintainability.
💡 Example refactor using a wrapper function
+const createVolumeIcon = (paths: string[]) => () => ( + <svg + xmlns="http://www.w3.org/2000/svg" + width="24" + height="24" + viewBox="0 0 24 24" + fill="var(--color-white)" + stroke="currentColor" + stroke-width="2" + stroke-linecap="round" + stroke-linejoin="round" + aria-hidden="true" + > + {paths.map(d => <path d={d} />)} + </svg> +); + const volumeIcons: IconSet = { - mute: () => ( - <svg - xmlns="http://www.w3.org/2000/svg" - width="24" - height="24" - viewBox="0 0 24 24" - fill="var(--color-white)" - stroke="currentColor" - stroke-width="2" - stroke-linecap="round" - stroke-linejoin="round" - aria-hidden="true" - > - <path d="M16 9a5 5 0 0 1 .95 2.293" /> - <path d="M19.364 5.636a9 9 0 0 1 1.889 9.96" /> - <path d="m2 2 20 20" /> - <path d="m7 7-.587.587A1.4 1.4 0 0 1 5.416 8H3a1 1 0 0 0-1 1v6a1 1 0 0 0 1 1h2.416a1.4 1.4 0 0 1 .997.413l3.383 3.384A.705.705 0 0 0 11 19.298V11" /> - <path d="M9.828 4.172A.686.686 0 0 1 11 4.657v.686" /> - </svg> - ), + mute: createVolumeIcon([ + "M16 9a5 5 0 0 1 .95 2.293", + "M19.364 5.636a9 9 0 0 1 1.889 9.96", + "m2 2 20 20", + "m7 7-.587.587A1.4 1.4 0 0 1 5.416 8H3a1 1 0 0 0-1 1v6a1 1 0 0 0 1 1h2.416a1.4 1.4 0 0 1 .997.413l3.383 3.384A.705.705 0 0 0 11 19.298V11", + "M9.828 4.172A.686.686 0 0 1 11 4.657v.686" + ]), // ... similar for other icons };
80-90: Logic is correct; consider extracting magic numbers.The icon selection logic correctly divides the volume range into appropriate tiers. However, the threshold values (33, 66) are hard-coded, which reduces maintainability if the ranges need adjustment.
💡 Suggested refactor with constants
+const VOLUME_THRESHOLDS = { + LOW: 33, + MEDIUM: 66, +} as const; + const getVolumeIcon = (volume: number, isMuted: boolean) => { if (isMuted || volume === 0) { return volumeIcons.mute; - } else if (volume > 0 && volume <= 33) { + } else if (volume > 0 && volume <= VOLUME_THRESHOLDS.LOW) { return volumeIcons.volumeLow; - } else if (volume > 33 && volume <= 66) { + } else if (volume > VOLUME_THRESHOLDS.LOW && volume <= VOLUME_THRESHOLDS.MEDIUM) { return volumeIcons.volumeMedium; } else { return volumeIcons.volumeHigh; } };js/hang-ui/src/Components/watch/FullscreenButton.tsx (1)
54-56: Consider a dynamic title for improved accessibility.The button's
title(and derivedaria-label) is always "Fullscreen", but the action toggles between entering and exiting. Screen reader users would benefit from knowing the current action.🔎 Suggested improvement
return ( - <Button title="Fullscreen" onClick={onClick}> + <Button title={context.isFullscreen() ? "Exit fullscreen" : "Enter fullscreen"} onClick={onClick}> {context.isFullscreen() ? fullscreenIcons.exit() : fullscreenIcons.enter()} </Button> );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockdeno.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
js/hang-ui/package.jsonjs/hang-ui/src/Components/publish/CameraSourceButton.tsxjs/hang-ui/src/Components/publish/FileSourceButton.tsxjs/hang-ui/src/Components/publish/MediaSourceSelector.tsxjs/hang-ui/src/Components/publish/MicrophoneSourceButton.tsxjs/hang-ui/src/Components/publish/NothingSourceButton.tsxjs/hang-ui/src/Components/publish/ScreenSourceButton.tsxjs/hang-ui/src/Components/publish/styles.cssjs/hang-ui/src/Components/shared/__tests__/components/index.test.tsxjs/hang-ui/src/Components/shared/components/button/index.tsxjs/hang-ui/src/Components/shared/components/button/style.cssjs/hang-ui/src/Components/shared/styles/flex.cssjs/hang-ui/src/Components/shared/styles/utils.tsjs/hang-ui/src/Components/shared/styles/variables.cssjs/hang-ui/src/Components/shared/types/icons.tsjs/hang-ui/src/Components/stats/__tests__/components/StatsItem.test.tsxjs/hang-ui/src/Components/stats/components/StatsPanel.tsxjs/hang-ui/src/Components/watch/FullscreenButton.tsxjs/hang-ui/src/Components/watch/PlayPauseButton.tsxjs/hang-ui/src/Components/watch/StatsButton.tsxjs/hang-ui/src/Components/watch/VolumeSlider.tsxjs/hang-ui/src/Components/watch/WatchControls.tsxjs/hang-ui/src/Components/watch/WatchUIContextProvider.tsxjs/hang-ui/src/Components/watch/styles.css
🚧 Files skipped from review as they are similar to previous changes (20)
- js/hang-ui/package.json
- js/hang-ui/src/Components/stats/tests/components/StatsItem.test.tsx
- js/hang-ui/src/Components/shared/styles/variables.css
- js/hang-ui/src/Components/publish/MicrophoneSourceButton.tsx
- js/hang-ui/src/Components/watch/styles.css
- js/hang-ui/src/Components/stats/components/StatsPanel.tsx
- js/hang-ui/src/Components/publish/MediaSourceSelector.tsx
- js/hang-ui/src/Components/publish/ScreenSourceButton.tsx
- js/hang-ui/src/Components/publish/FileSourceButton.tsx
- js/hang-ui/src/Components/shared/components/button/index.tsx
- js/hang-ui/src/Components/publish/CameraSourceButton.tsx
- js/hang-ui/src/Components/shared/types/icons.ts
- js/hang-ui/src/Components/shared/styles/flex.css
- js/hang-ui/src/Components/shared/components/button/style.css
- js/hang-ui/src/Components/shared/tests/components/index.test.tsx
- js/hang-ui/src/Components/publish/styles.css
- js/hang-ui/src/Components/shared/styles/utils.ts
- js/hang-ui/src/Components/publish/NothingSourceButton.tsx
- js/hang-ui/src/Components/watch/StatsButton.tsx
- js/hang-ui/src/Components/watch/WatchControls.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
js/hang-ui/src/Components/watch/VolumeSlider.tsx (2)
js/hang-ui/src/Components/shared/types/icons.ts (1)
IconSet(6-6)js/hang-ui/src/Components/shared/components/button/index.tsx (1)
Button(44-68)
js/hang-ui/src/Components/watch/PlayPauseButton.tsx (3)
js/hang-ui/src/Components/shared/types/icons.ts (1)
IconSet(6-6)js/hang-ui/src/Components/watch/useWatchUIContext.ts (1)
useWatchUIContext(4-12)js/hang-ui/src/Components/shared/components/button/index.tsx (1)
Button(44-68)
js/hang-ui/src/Components/watch/FullscreenButton.tsx (3)
js/hang-ui/src/Components/shared/types/icons.ts (1)
IconSet(6-6)js/hang-ui/src/Components/watch/useWatchUIContext.ts (1)
useWatchUIContext(4-12)js/hang-ui/src/Components/shared/components/button/index.tsx (1)
Button(44-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
🔇 Additional comments (4)
js/hang-ui/src/Components/watch/VolumeSlider.tsx (2)
2-3: LGTM!The imports are correctly structured and the shared Button component and IconSet type are appropriately utilized throughout the file.
109-111: Correct Button integration with proper accessibility.The Button component is properly integrated with dynamic title/aria-label based on mute state. The
getVolumeIcon(...)()pattern correctly follows the IconSet architecture where icon functions are returned and then invoked to produce JSX.js/hang-ui/src/Components/watch/PlayPauseButton.tsx (1)
1-51: LGTM!The migration to the shared
Buttoncomponent is clean. SVGs correctly usearia-hidden="true"since theButtoncomponent derivesaria-labelfrom thetitleprop. The dynamic icon selection based onisPlaying()state is straightforward.js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (1)
59-65: Fullscreen toggle logic is correctly implemented.The race condition from the previous review has been addressed. The
toggleFullscreenfunction now only triggers the DOM API, and thefullscreenchangeevent handler (lines 179-181) properly synchronizes theisFullscreensignal with the actual fullscreen state.Also applies to: 179-181
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
js/hang-ui/src/Components/shared/components/button/style.css (1)
16-21: Remove redundant color property in hover state.Line 18 sets
color: var(--color-white);, but this is already defined in the base.buttonrule (line 5) and doesn't change on hover. The redundant declaration adds noise without serving a purpose.🔎 Proposed fix
.button:hover:not(:disabled) { transform: scale(1.02); - color: var(--color-white); background-color: var(--color-white-alpha-20); box-shadow: 0 var(--spacing-2) var(--spacing-8) var(--color-black-alpha-10); }js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (1)
59-65: Consider adding error handling for fullscreen API calls.The fullscreen implementation correctly uses event-driven state updates to avoid race conditions, and the cleanup is properly handled with
onCleanup. The past critical issues have been addressed.However,
exitFullscreen()andrequestFullscreen()can fail (e.g., user denial, browser restrictions, permission issues). While the state will remain accurate becausefullscreenchangeonly fires on successful transitions, unhandled promise rejections could still surface in the console or cause unexpected behavior.🔎 Optional: Add error handling
const toggleFullscreen = () => { if (document.fullscreenElement) { - document.exitFullscreen(); + document.exitFullscreen().catch((err) => { + console.warn("Failed to exit fullscreen:", err); + }); } else { - props.hangWatch.requestFullscreen(); + props.hangWatch.requestFullscreen().catch((err) => { + console.warn("Failed to enter fullscreen:", err); + }); } };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockdeno.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
js/hang-ui/src/Components/shared/components/button/style.cssjs/hang-ui/src/Components/shared/styles/flex.cssjs/hang-ui/src/Components/watch/WatchUIContextProvider.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
🔇 Additional comments (1)
js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (1)
179-187: LGTM: Fullscreen event listener and cleanup correctly implemented.The
fullscreenchangelistener is properly registered increateEffectand cleaned up using SolidJS'sonCleanup. This ensures the state stays in sync with the actual fullscreen status and prevents memory leaks. The past critical issue regarding SolidJS cleanup patterns has been correctly addressed.
| :is(.flex) { | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| } | ||
|
|
||
| :is(.flex--center) { | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate utility classes.
.flex and .flex--center are completely identical. This duplication is unnecessary and will confuse maintainers about which class to use.
Consider one of these approaches:
- Remove
.flex--centerentirely and use only.flex - Make
.flexa base class with justdisplay: flex;and.flex--centera modifier that adds centering (requiring both classes:class="flex flex--center") - If both are needed as standalone utilities, rename them to clarify their distinct purposes (though they currently have none)
Note: If you choose approach 2, .flex--align-center (lines 13-15) should also be updated to follow the same modifier pattern.
🔎 Proposed fix (approach 1 - remove duplication)
:is(.flex) {
display: flex;
justify-content: center;
align-items: center;
}
-:is(.flex--center) {
- display: flex;
- justify-content: center;
- align-items: center;
-}
-
:is(.flex--align-center) {
+ display: flex;
align-items: center;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In js/hang-ui/src/Components/shared/styles/flex.css around lines 1-11, the .flex
and .flex--center rules are identical; convert to a base/modifier pattern: make
.flex contain only display: flex; make .flex--center contain justify-content:
center and align-items: center (so consumers use class="flex flex--center");
also update the related .flex--align-center rule (lines ~13-15) to be a modifier
matching this pattern (remove duplicate full definitions), and remove any
redundant identical blocks so only the base + modifiers remain.
| import type { IconSet } from "../shared/types/icons"; | ||
| import useWatchUIContext from "./useWatchUIContext"; | ||
|
|
||
| const fullscreenIcons: IconSet = { |
There was a problem hiding this comment.
Could we make these files on disk instead of inlined Javascript?
Either the bundler could inline them (meh) or they could be packaged into a public folder that would need to be served by the application (cachable).
pzanella
force-pushed
the
feat/button-icons
branch
from
15d5fd4 to
90d55f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
js/hang-ui/src/themes/flex.css (1)
1-9: Consider simplifying:is()usage.The
:is()pseudo-class is typically used for grouping multiple selectors, but here it wraps only a single class selector. This adds unnecessary complexity without benefit.🔎 Proposed simplification
-:is(.flex--center) { +.flex--center { display: flex; justify-content: center; align-items: center; } -:is(.flex--align-center) { +.flex--align-center { align-items: center; }js/hang-ui/src/Components/watch/VolumeSlider.tsx (1)
6-16: Clarify volume threshold boundaries.The volume icon selection logic is clear, but the boundary conditions could be more explicit. Currently:
volume <= 33→ lowvolume > 33 && volume <= 66→ medium- else → high
Consider whether
volume === 33should map to low or medium (currently low). The logic is correct but documenting the inclusive/exclusive boundaries would improve maintainability.Optional: Add boundary clarification comment
const getVolumeIcon = (volume: number, isMuted: boolean) => { if (isMuted || volume === 0) { return <Icon name="mute" />; - } else if (volume > 0 && volume <= 33) { + } else if (volume > 0 && volume <= 33) { // 0-33: low return <Icon name="volume-low" />; - } else if (volume > 33 && volume <= 66) { + } else if (volume > 33 && volume <= 66) { // 34-66: medium return <Icon name="volume-medium" />; - } else { + } else { // 67-100: high return <Icon name="volume-high" />; } };js/hang-ui/src/utilities/loadStyle.ts (2)
35-37: Error is swallowed silently; callers won't know if CSS failed to load.The
catchblock logs the error but doesn't rethrow, so the returned promise resolves successfully even when the stylesheet fails to load. This may cause components to render without their expected styles, with no indication to the caller.Consider rethrowing or returning a status so callers can handle failures:
🔎 Proposed fix to propagate errors
} catch (error) { console.error(`[hang-ui] Error loading CSS from ${href}:`, error); + throw error; }
29-34: Consider adding a timeout to prevent indefinite hangs.The promise waits for
loadorerrorevents without a timeout. On slow or stalled networks, this could block indefinitely. For resilience, consider adding a timeout (e.g., 10 seconds).js/hang-ui/src/Components/shared/components/button/index.tsx (1)
28-37: JSDoc mentions "Constructable Stylesheet" but the implementation doesn't use one.Line 31 states the component "Injects a Constructable Stylesheet for style encapsulation", but the implementation is a simple button without any stylesheet injection. This documentation is misleading.
🔎 Suggested fix to remove inaccurate JSDoc claim
/** * Shared, accessible, and stylable Button component for SolidJS. * - * - Injects a Constructable Stylesheet for style encapsulation (Shadow DOM-friendly). * - Supports accessibility attributes and keyboard navigation. * - Accepts custom content, classes, and state modifiers (e.g., active, disabled). *js/hang-ui/src/Components/shared/components/icon/index.tsx (1)
45-93: Potential stale state update if icon name changes rapidly.When
props.namechanges, a new effect runs but doesn't cancel the previous fetch. If the old fetch completes after a newer one, it could overwrite the correct SVG. Consider tracking the current icon name to discard stale responses:🔎 Proposed fix using a cancellation flag
createEffect(() => { const iconName = props.name; + let cancelled = false; // Use cached SVG if available const cached = iconCache.get(iconName); if (cached) { setSvg(cached); setLoading(false); setError(false); return; } // ... fetch logic ... // Update state when fetch completes void fetchPromise .then((svgContent) => { + if (cancelled) return; setSvg(svgContent); setLoading(false); setError(false); }) .catch((err) => { + if (cancelled) return; console.error(`Error loading icon "${iconName}":`, err); setLoading(false); setError(true); }) .finally(() => { fetchingIcons.delete(iconName); }); + + onCleanup(() => { + cancelled = true; + }); });js/hang-ui/src/utilities/basePath.ts (1)
6-14: Script detection selector could match unintended scripts.The selector
script[src*="hang-ui"]matches any script containing "hang-ui" anywhere in the src. This could incorrectly match scripts likenot-hang-ui.jsorhang-ui-fork.js.🔎 Suggested more precise selector
- const script = document.querySelector<HTMLScriptElement>('script[src*="hang-ui"]'); + const script = document.querySelector<HTMLScriptElement>('script[src*="/hang-ui/"]') + ?? document.querySelector<HTMLScriptElement>('script[src$="hang-ui.js"]');Alternatively, consider using a more specific attribute like
data-hang-uion the script tag.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (22)
bun.lockis excluded by!**/*.lockdeno.lockis excluded by!**/*.lockjs/hang-ui/src/assets/icons/arrow-down.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/arrow-up.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/audio.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/ban.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/buffer.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/camera.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/file.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/fullscreen-enter.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/fullscreen-exit.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/microphone.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/mute.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/network.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/pause.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/play.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/screen.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/stats.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/video.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/volume-high.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/volume-low.svgis excluded by!**/*.svgjs/hang-ui/src/assets/icons/volume-medium.svgis excluded by!**/*.svg
📒 Files selected for processing (43)
js/hang-demo/src/index.tsjs/hang-demo/src/meet.tsjs/hang-demo/src/publish.tsjs/hang-demo/src/vite-env.d.tsjs/hang-demo/vite.config.tsjs/hang-ui/README.mdjs/hang-ui/package.jsonjs/hang-ui/rollup.config.mjsjs/hang-ui/src/Components/publish/CameraSourceButton.tsxjs/hang-ui/src/Components/publish/FileSourceButton.tsxjs/hang-ui/src/Components/publish/MediaSourceSelector.tsxjs/hang-ui/src/Components/publish/MicrophoneSourceButton.tsxjs/hang-ui/src/Components/publish/NothingSourceButton.tsxjs/hang-ui/src/Components/publish/ScreenSourceButton.tsxjs/hang-ui/src/Components/publish/element.tsxjs/hang-ui/src/Components/shared/__tests__/components/button/index.test.tsxjs/hang-ui/src/Components/shared/__tests__/components/icon/index.test.tsxjs/hang-ui/src/Components/shared/components/button/index.tsxjs/hang-ui/src/Components/shared/components/icon/index.tsxjs/hang-ui/src/Components/stats/Stats.tsxjs/hang-ui/src/Components/stats/__tests__/Stats.test.tsxjs/hang-ui/src/Components/stats/__tests__/components/StatsItem.test.tsxjs/hang-ui/src/Components/stats/__tests__/components/StatsPanel.test.tsxjs/hang-ui/src/Components/stats/components/StatsItem.tsxjs/hang-ui/src/Components/stats/components/StatsPanel.tsxjs/hang-ui/src/Components/stats/variables.cssjs/hang-ui/src/Components/watch/FullscreenButton.tsxjs/hang-ui/src/Components/watch/PlayPauseButton.tsxjs/hang-ui/src/Components/watch/StatsButton.tsxjs/hang-ui/src/Components/watch/VolumeSlider.tsxjs/hang-ui/src/Components/watch/WatchControls.tsxjs/hang-ui/src/Components/watch/WatchUIContextProvider.tsxjs/hang-ui/src/Components/watch/element.tsxjs/hang-ui/src/css.d.tsjs/hang-ui/src/themes/button.cssjs/hang-ui/src/themes/flex.cssjs/hang-ui/src/themes/publish/styles.cssjs/hang-ui/src/themes/stats.cssjs/hang-ui/src/themes/variables.cssjs/hang-ui/src/themes/watch/styles.cssjs/hang-ui/src/utilities/basePath.tsjs/hang-ui/src/utilities/index.tsjs/hang-ui/src/utilities/loadStyle.ts
💤 Files with no reviewable changes (3)
- js/hang-ui/src/Components/stats/Stats.tsx
- js/hang-ui/src/Components/stats/variables.css
- js/hang-ui/src/css.d.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- js/hang-ui/src/Components/stats/tests/components/StatsItem.test.tsx
- js/hang-ui/package.json
- js/hang-ui/src/Components/publish/FileSourceButton.tsx
- js/hang-ui/src/Components/publish/NothingSourceButton.tsx
- js/hang-ui/src/Components/watch/PlayPauseButton.tsx
- js/hang-ui/src/Components/publish/MicrophoneSourceButton.tsx
- js/hang-ui/src/Components/publish/CameraSourceButton.tsx
🧰 Additional context used
🧬 Code graph analysis (15)
js/hang-ui/src/Components/watch/VolumeSlider.tsx (2)
js/hang-ui/src/Components/shared/components/icon/index.tsx (1)
Icon(37-106)js/hang-ui/src/Components/shared/components/button/index.tsx (1)
Button(38-55)
js/hang-ui/src/Components/shared/__tests__/components/icon/index.test.tsx (4)
js/hang-ui/src/utilities/basePath.ts (1)
setBasePath(39-45)js/hang-ui/src/utilities/index.ts (1)
setBasePath(4-4)js/hang-ui/src/Components/shared/components/icon/index.tsx (1)
Icon(37-106)js/signals/src/dom.ts (1)
render(68-89)
js/hang-ui/src/Components/shared/__tests__/components/button/index.test.tsx (2)
js/hang-ui/src/Components/shared/components/button/index.tsx (2)
ButtonProps(16-26)Button(38-55)js/signals/src/dom.ts (1)
render(68-89)
js/hang-ui/src/utilities/loadStyle.ts (2)
js/hang-ui/src/utilities/index.ts (3)
loadStyleIntoShadow(4-4)whenBasePathReady(4-4)getBasePath(4-4)js/hang-ui/src/utilities/basePath.ts (2)
whenBasePathReady(52-57)getBasePath(66-71)
js/hang-ui/src/Components/watch/FullscreenButton.tsx (3)
js/hang-ui/src/Components/watch/useWatchUIContext.ts (1)
useWatchUIContext(4-12)js/hang-ui/src/Components/shared/components/button/index.tsx (1)
Button(38-55)js/hang-ui/src/Components/shared/components/icon/index.tsx (1)
Icon(37-106)
js/hang-ui/src/Components/shared/components/icon/index.tsx (1)
js/hang-ui/src/utilities/basePath.ts (1)
getBasePath(66-71)
js/hang-ui/src/Components/watch/element.tsx (2)
js/hang-ui/src/utilities/index.ts (1)
loadStyleIntoShadow(4-4)js/hang-ui/src/utilities/loadStyle.ts (1)
loadStyleIntoShadow(11-38)
js/hang-ui/rollup.config.mjs (1)
js/signals/src/solid.ts (1)
solid(5-11)
js/hang-demo/src/publish.ts (2)
js/hang-ui/src/utilities/basePath.ts (1)
setBasePath(39-45)js/hang-ui/src/utilities/index.ts (1)
setBasePath(4-4)
js/hang-ui/src/utilities/basePath.ts (1)
js/hang-ui/src/utilities/index.ts (3)
setBasePath(4-4)whenBasePathReady(4-4)getBasePath(4-4)
js/hang-ui/src/Components/publish/element.tsx (2)
js/hang-ui/src/utilities/index.ts (1)
loadStyleIntoShadow(4-4)js/hang-ui/src/utilities/loadStyle.ts (1)
loadStyleIntoShadow(11-38)
js/hang-demo/src/meet.ts (2)
js/hang-ui/src/utilities/basePath.ts (1)
setBasePath(39-45)js/hang-ui/src/utilities/index.ts (1)
setBasePath(4-4)
js/hang-ui/src/Components/stats/components/StatsPanel.tsx (3)
js/hang-ui/src/Components/stats/types.ts (1)
KnownStatsProviders(1-1)js/hang-ui/src/Components/stats/components/StatsItem.tsx (1)
StatsItem(19-51)js/hang-ui/src/Components/shared/components/icon/index.tsx (1)
Icon(37-106)
js/hang-ui/src/Components/publish/ScreenSourceButton.tsx (2)
js/hang-ui/src/Components/shared/components/button/index.tsx (1)
Button(38-55)js/hang-ui/src/Components/shared/components/icon/index.tsx (1)
Icon(37-106)
js/hang-demo/src/index.ts (2)
js/hang-ui/src/utilities/basePath.ts (1)
setBasePath(39-45)js/hang-ui/src/utilities/index.ts (1)
setBasePath(4-4)
🪛 markdownlint-cli2 (0.18.1)
js/hang-ui/README.md
84-84: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (37)
js/hang-ui/src/Components/stats/__tests__/Stats.test.tsx (1)
3-3: LGTM! Clean test mocking setup.The addition of fetch mocking prevents real network requests for Icon SVG files during tests, and proper cleanup is ensured with
vi.restoreAllMocks()inafterEach. This aligns well with the Icon component integration.Also applies to: 16-20, 26-26
js/hang-ui/src/themes/variables.css (1)
1-32: LGTM! Well-structured CSS custom properties.The theming system establishes a comprehensive color palette and spacing scale using CSS custom properties within a
:hostblock, which is appropriate for web component styling. The spacing scale follows a logical progression with helpful pixel-value comments.js/hang-ui/src/themes/watch/styles.css (2)
1-4: LGTM! Centralized theme imports.The import statements properly consolidate shared styling from variables, flex utilities, button themes, and stats themes, enabling consistent theming across the watch UI.
21-67: LGTM! Sophisticated playback button styling.The gradient-based playback button with layered pseudo-element effects creates polished hover and active states. The use of CSS custom properties from
variables.cssensures consistency with the design system, and the transitions provide smooth visual feedback.js/hang-ui/src/Components/watch/WatchControls.tsx (1)
12-12: LGTM! Clean utility class addition.The
flex--align-centerclass provides vertical alignment for the playback controls row, consistent with the utility-driven CSS approach introduced across the theme system.js/hang-ui/src/Components/stats/__tests__/components/StatsPanel.test.tsx (1)
2-2: LGTM! Consistent test mocking pattern.The fetch mocking setup matches the pattern in
Stats.test.tsx, preventing real network requests for Icon SVG files and ensuring proper cleanup. This consistency across the test suite is good practice.Also applies to: 17-21, 28-28
js/hang-demo/src/publish.ts (1)
2-4: LGTM! Correct asset base path initialization.The
setBasePathcall is properly positioned before importing Hang UI elements, ensuring the assets base path is configured before any asset loading occurs. The use of the ambient__HANG_UI_ASSETS_PATH__global aligns with the Vite configuration setup.js/hang-demo/src/meet.ts (1)
2-4: LGTM! Consistent base path setup.The asset base path initialization follows the same pattern as
publish.ts, withsetBasePathcalled before importing Hang UI elements. This consistency ensures predictable asset resolution across the demo application.js/hang-ui/src/Components/watch/element.tsx (1)
5-5: LGTM! Clean transition to runtime CSS injection.The implementation correctly loads styles dynamically before mounting inner elements, with proper fallback handling for the shadow root.
Also applies to: 16-18
js/hang-demo/src/vite-env.d.ts (1)
10-11: LGTM! Standard Vite global constant declaration.The ambient type declaration properly supports the runtime asset path configuration.
js/hang-ui/src/Components/stats/components/StatsItem.tsx (1)
44-44: Flag inconsistency between code and summary.The AI summary states that this component now uses the shared
Iconcomponent, but the code still rendersprops.svgas a JSX.Element. The props interface also still declaressvg: JSX.Elementrather than an icon name string.js/hang-ui/src/Components/watch/FullscreenButton.tsx (1)
1-2: LGTM! Excellent refactor to shared components.The implementation cleanly delegates fullscreen logic to the context and leverages the shared Button and Icon components for consistent UI. The dynamic icon selection based on state is well done.
Also applies to: 9-9, 13-15
js/hang-demo/src/index.ts (1)
2-2: LGTM! Proper base path initialization.The base path is correctly set before importing any hang-ui components, ensuring assets load from the right location.
Also applies to: 4-4
js/hang-ui/src/Components/shared/__tests__/components/icon/index.test.tsx (1)
18-78: Good test coverage for Icon component behavior.The tests comprehensively cover success, error, and caching scenarios with appropriate assertions on DOM state and fetch call counts.
js/hang-ui/README.md (1)
51-105: LGTM! Excellent documentation for asset configuration.The Asset Configuration section provides clear guidance on auto-detection, manual setup, required directory structure, and utility functions. The examples cover common use cases effectively.
js/hang-ui/src/utilities/index.ts (1)
1-4: LGTM!Clean barrel export pattern for utilities that support base path resolution and runtime style loading.
js/hang-ui/src/themes/publish/styles.css (2)
1-3: LGTM!The import order correctly establishes the cascade: variables → layout utilities → component styles.
38-43: Icon assets verified—all required icons exist at expected paths.The button--media-source-selector modifier appropriately resets button styles for the inline toggle control. Verification confirms all icon assets referenced in the components exist:
- arrow-up.svg and arrow-down.svg referenced by MediaSourceSelector ✓
- screen.svg referenced by ScreenSourceButton ✓
The Icon component correctly resolves icon names to paths in
assets/icons/, and all referenced files are present.js/hang-ui/src/Components/shared/__tests__/components/button/index.test.tsx (1)
1-60: LGTM!Comprehensive test suite covering all Button component props and behaviors, including ARIA attributes, disabled state, click handling, and JSX children composition. This provides solid coverage for the shared Button component.
js/hang-ui/src/Components/publish/ScreenSourceButton.tsx (1)
1-24: LGTM!Clean migration to the shared Button and Icon components while preserving the existing active state logic and onClick behavior.
js/hang-ui/src/Components/publish/MediaSourceSelector.tsx (1)
1-38: LGTM!The migration to Button and Icon components is well-implemented. The dynamic icon switching based on
sourcesVisible()provides clear visual feedback, and the CSS class correctly applies the custom button--media-source-selector styling.js/hang-ui/src/Components/publish/element.tsx (1)
5-15: Code changes are correct; setBasePath is properly initialized before component instantiation.The runtime CSS loading implementation correctly awaits the stylesheet and handles both shadow DOM and light DOM scenarios. Verification confirms that
setBasePath(__HANG_UI_ASSETS_PATH__)is called in all application entry points (js/hang-demo/src/publish.ts, meet.ts, index.ts) at module load time, before importing the hang-publish-ui component. TheloadStyleIntoShadowfunction properly waits forwhenBasePathReady()before resolving the asset path, ensuring basePath is initialized before CSS loading attempts.js/hang-ui/src/Components/watch/StatsButton.tsx (1)
1-20: Looks good!Clean migration to the shared Button and Icon components. The removal of inline SVG in favor of the Icon component improves maintainability. The dynamic title correctly reflects the panel visibility state for accessibility, and the stats icon asset is properly available.
js/hang-ui/src/Components/watch/VolumeSlider.tsx (1)
35-37: Icon assets verified and properly configured.All volume icon assets referenced by
getVolumeIcon()are present:mute.svg,volume-low.svg,volume-medium.svg, andvolume-high.svginjs/hang-ui/src/assets/icons/. The Icon component correctly resolves these names to SVG files at runtime with proper error handling and caching. The Button integration and dynamic accessibility title are clean and functional.js/hang-demo/vite.config.ts (1)
9-12: LGTM! Dev asset path injection is correctly configured.The
/@fsprefix is the correct Vite mechanism for serving files outside the project root during development, andJSON.stringifyproperly escapes the path string.Ensure production builds configure
__HANG_UI_ASSETS_PATH__appropriately (e.g., via environment variable or bundler define), since this config only applies to development.js/hang-ui/src/Components/watch/WatchUIContextProvider.tsx (2)
59-65: Fullscreen toggle correctly delegates state updates to the event listener.Previous race condition is resolved by removing the synchronous
setIsFullscreencall. State now updates reactively via thefullscreenchangeevent handler at Lines 179-181.
178-187: Proper cleanup withonCleanupfor the fullscreenchange listener.Previous issue with SolidJS cleanup is correctly addressed. The event listener is properly registered and removed using
onCleanup.js/hang-ui/src/Components/stats/components/StatsPanel.tsx (2)
1-2: Clean refactor to use the shared Icon component.Replacing inline SVGs with the Icon component improves maintainability and enables centralized icon management with caching.
41-48: Icon integration with StatsItem looks correct.The
svgprop receives a JSX element (<Icon name={iconName} />), which aligns withStatsItem's expectedprops.svgusage shown in the relevant code snippets.js/hang-ui/src/Components/shared/components/button/index.tsx (1)
38-54: Button implementation is clean and accessible.Good accessibility patterns with proper
aria-labelfallback chain andaria-disabledhandling. The component correctly passes through native button attributes.js/hang-ui/src/Components/shared/components/icon/index.tsx (1)
95-105: UsinginnerHTMLfor SVG injection—ensure assets are trusted.Injecting raw HTML via
innerHTMLis safe here since SVGs are loaded from your own asset path. Just ensure these assets remain under your control and aren't user-provided.js/hang-ui/src/utilities/basePath.ts (1)
66-71:getBasePath()returns empty string if basePath is unset.If auto-detection fails and
setBasePathhasn't been called,getBasePath("assets/foo.svg")returns"/assets/foo.svg"(relative path). This may work in some contexts but could fail in others. Consider logging a warning or throwing if basePath is empty when a subpath is requested.Verify that consumers always call
setBasePathor that auto-detection reliably succeeds beforegetBasePathis invoked. The Icon component doesn't wait forwhenBasePathReady(), which could trigger this scenario.js/hang-ui/rollup.config.mjs (3)
20-30: Good refactor to extract shared plugins.Centralizing the plugin configuration in
sharedPluginsimproves maintainability and ensures consistency across component builds.
32-51: Utilities build correctly excludes JSX processing.The utilities config uses a narrower
includepattern (/\.[jt]s$/) since it contains only TypeScript without JSX. Thecopyplugin withcopyOnce: trueefficiently handles asset copying.
53-70: Component builds properly reuse sharedPlugins.Both publish and watch component configs correctly leverage the shared plugin setup, ensuring consistent build behavior.
js/hang-ui/src/themes/stats.css (2)
96-142: LGTM!The per-item color theming is well-organized with consistent patterns across network, video, audio, and buffer categories. The use of semantic color tokens (e.g.,
color-blue-400,color-purple-900) aligns with design system best practices.
144-160: LGTM!Token migrations for spacing and color are consistent with the rest of the file.
| beforeEach(() => { | ||
| // Set a fake base path for tests | ||
| setBasePath("/test-assets"); | ||
| // Clear caches between tests | ||
| // @ts-expect-error: access for test | ||
| Icon.__iconCache?.clear?.(); | ||
| // @ts-expect-error: access for test | ||
| Icon.__fetchingIcons?.clear?.(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add public test utilities instead of using @ts-expect-error.
Accessing private properties via @ts-expect-error for test cleanup is a testing anti-pattern. It couples tests to implementation details and will break silently if the property names change.
Consider adding a public test utility function to the Icon module:
// In icon/index.tsx
export function resetIconCacheForTesting(): void {
iconCache.clear();
fetchingIcons.clear();
}Then use it in tests:
import Icon, { resetIconCacheForTesting } from "../../../../shared/components/icon";
beforeEach(() => {
setBasePath("/test-assets");
resetIconCacheForTesting();
});This provides a stable, type-safe testing API that won't break silently.
🤖 Prompt for AI Agents
js/hang-ui/src/Components/shared/__tests__/components/icon/index.test.tsx lines
8-16: tests are accessing private Icon internals with @ts-expect-error to clear
caches; add a public test utility in the icon module that clears the internal
caches (e.g., export resetIconCacheForTesting which clears iconCache and
fetchingIcons) and update this test to import and call that function after
setBasePath, removing the @ts-expect-error lines; ensure the exported function
is typed and only used in tests (document or export from test-only entry if
desired).
|
Hi @kixelated, |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
js/hang-ui/src/themes/stats.css (1)
64-82: Critical issue resolved; consider minor consolidation.The duplicate rule that was overriding the responsive behavior has been successfully removed. The icon wrapper will now correctly hide by default and only display at viewports wider than 480px.
However, lines 68-69 set
width: fit-contentandheight: fit-content, which are immediately overridden by24pxin the container query (lines 75-76). For cleaner code, consider setting the fixed dimensions in the base rule as originally suggested.🔎 Optional consolidation
.stats .stats__panel .stats__item .stats__icon-wrapper { display: none; padding: var(--spacing-8); border-radius: 8px; - width: fit-content; - height: fit-content; + width: 24px; + height: 24px; flex-shrink: 0; } @container (width > 480px) { .stats .stats__panel .stats__item .stats__icon-wrapper { - width: 24px; - height: 24px; display: flex; align-items: center; justify-content: center; flex-direction: row; } }js/hang-ui/src/Components/shared/components/icon/index.tsx (1)
14-23: Consider cache size management for long-running applications.The global
iconCachenever evicts entries, which could accumulate memory in long-running applications that load many different icons over time. For typical applications with a fixed icon set, this is likely acceptable. However, if the application dynamically loads many unique icons, consider implementing a cache size limit or LRU eviction strategy.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
js/hang-ui/README.mdjs/hang-ui/src/Components/shared/__tests__/components/icon/index.test.tsxjs/hang-ui/src/Components/shared/components/icon/index.tsxjs/hang-ui/src/themes/button.cssjs/hang-ui/src/themes/stats.css
🚧 Files skipped from review as they are similar to previous changes (2)
- js/hang-ui/README.md
- js/hang-ui/src/Components/shared/tests/components/icon/index.test.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
js/hang-ui/src/Components/shared/components/icon/index.tsx (1)
js/hang-ui/src/utilities/basePath.ts (2)
whenBasePathReady(52-57)getBasePath(66-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
🔇 Additional comments (9)
js/hang-ui/src/themes/stats.css (2)
84-87: LGTM!Setting SVG dimensions to 100% ensures the icons properly fill the 24px wrapper, maintaining consistent icon sizing across the stats panel.
21-27: LGTM!The design token updates are consistent throughout:
- Spacing tokens properly applied for padding, gap, and layout calculations
- Color tokens follow a cohesive pattern (e.g., blue-400 foreground with blue-900 background)
- Responsive layout adjustments align with the container query breakpoints
Also applies to: 35-35, 42-42, 52-62, 89-135, 142-149
js/hang-ui/src/themes/button.css (4)
1-11: LGTM!The base button styles are well-structured with appropriate use of CSS custom properties and smooth transitions.
13-18: LGTM!The hover state provides good visual feedback with appropriate transform and shadow effects while correctly excluding disabled buttons.
20-23: LGTM!The active state provides appropriate "pressed" visual feedback that complements the hover state.
25-29: LGTM!The disabled state is well-implemented with appropriate visual feedback. The previous issue with the undefined
--color-light-grayvariable has been successfully addressed.js/hang-ui/src/Components/shared/components/icon/index.tsx (3)
1-12: LGTM!The imports and type definitions are clean and well-documented. The IconProps interface is appropriately simple.
96-107: Verify icon sources for innerHTML security.The component uses
innerHTMLto inject SVG content from fetched files. While this is safe when loading from a controlled assets directory, ensure that:
- The asset path cannot be manipulated to load untrusted content
- Icon SVG files are part of the build process and not user-uploaded
The current implementation using
getBasePath()with a fixed path structure appears safe.
101-101: Accessibility: Icons marked as decorative.The component sets
aria-hidden={true}, treating all icons as decorative (ignored by screen readers). This is appropriate if icons are purely visual and any semantic meaning is conveyed through surrounding text or ARIA labels on parent elements. If icons ever need to convey meaning independently, consider adding an optionalaria-labelprop.
2 participants
Introduce shared UI components folder
This pull request creates the folder directory to organize common UI components. Centralizing shared elements improves code reuse, maintainability, and consistency across the user interface.