feat: add shared/button component #793
Conversation
WalkthroughAdds a shared Button component (with exported ButtonProps) and tests, a lightweight icon type system (Icon, IconSet, SVGIcons), and a useConstructableStyle utility. Introduces shared CSS: variables, flex utilities, and Button stylesheet. Replaces many native elements with the new Button and swaps emoji labels for inline SVG icons (SVGs marked aria-hidden where applied). Watch UI context now exposes isFullscreen and toggleFullscreen. Two devDependencies were added to package.json. Removes legacy .publishButton and .watchControlButton CSS rules. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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)
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).
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.