feat: restyling Publish UI #857
Conversation
WalkthroughThe PR renames CSS classes across publish UI components to a BEM-like 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In `@js/hang-ui/src/publish/components/NothingSourceButton.tsx`:
- Around line 14-18: The "No Source" button in NothingSourceButton is missing
the shared wrapper class used by other source buttons; update the component so
the Button is wrapped (or the outer div) uses the
publish-ui__source-button-wrapper class to ensure spacing/margins from
source-button.css are applied (locate the JSX in the NothingSourceButton
component where the Button with class publish-ui__source-button--no-source is
rendered and add the publish-ui__source-button-wrapper wrapper class to match
other source buttons).
🧹 Nitpick comments (4)
js/hang-ui/src/publish/components/FileSourceButton.tsx (1)
29-35: Redundantflex--centerclass.The
Buttoncomponent already appliesflex--centerinternally (seebutton.tsxline 42:class={`flex--center button ${props.class}`}). Adding it again in theclassprop results in duplicate class application.Suggested fix
<Button title="Upload File" - class={`publish-ui__source-button flex--center ${context.fileActive() ? "publish-ui__source-button--active" : ""}`} + class={`publish-ui__source-button ${context.fileActive() ? "publish-ui__source-button--active" : ""}`} onClick={onClick} >js/hang-ui/src/publish/components/ScreenSourceButton.tsx (1)
15-21: Redundantflex--centerclass.Same as
FileSourceButton: theButtoncomponent already appliesflex--centerinternally. Adding it in theclassprop is redundant.Suggested fix
<Button title="Screen" - class={`publish-ui__source-button flex--center ${context.screenActive() ? "publish-ui__source-button--active" : ""}`} + class={`publish-ui__source-button ${context.screenActive() ? "publish-ui__source-button--active" : ""}`} onClick={onClick} >js/hang-ui/src/publish/components/PublishStatusIndicator.tsx (1)
21-35: Consider memoizingstatusConfigto avoid repeated calls.
statusConfig()is invoked three times in the JSX (lines 29, 32, 34). In Solid.js, each call re-executes the function. While the computation is lightweight, you can avoid redundancy by usingcreateMemoor extracting the result once.♻️ Suggested optimization
+import { createMemo } from "solid-js"; import type { PublishStatus } from "../context"; import usePublishUIContext from "../hooks/use-publish-ui"; // ... STATUS_MAP and unknownStatus unchanged ... export default function PublishStatusIndicator() { const context = usePublishUIContext(); - const statusConfig = (): StatusIndicatorConfig => { + const statusConfig = createMemo((): StatusIndicatorConfig => { const status: PublishStatus = context.publishStatus(); return STATUS_MAP[status] || unknownStatus; - }; + }); return ( <div class="publish-ui__status-indicator flex--center"> <span class={`publish-ui__status-indicator-dot publish-ui__status-indicator-dot--${statusConfig().variant}`} /> <span class={`publish-ui__status-indicator-text publish-ui__status-indicator-text--${statusConfig().variant}`} > {statusConfig().text} </span> </div> ); }js/hang-ui/src/publish/styles/status-indicator.css (1)
19-41: Consolidate duplicate::afterselectors for clarity.Lines 19-29 define the
::afterpseudo-element with animation, then lines 31-33 immediately override it withdisplay: none. While this works via CSS cascade, it's confusing to read. Consider consolidating into a single base rule.♻️ Suggested consolidation
.publish-ui__status-indicator-dot::after { content: ""; position: absolute; inset: 0; border-radius: 50%; background: currentColor; transform: scale(1); opacity: 0.5; animation: status-pulse 1.5s ease-out infinite; z-index: -1; + display: none; } -.publish-ui__status-indicator-dot::after { - display: none; -} - .publish-ui__status-indicator-dot--live::after, .publish-ui__status-indicator-dot--warning::after, .publish-ui__status-indicator-dot--connecting::after, .publish-ui__status-indicator-dot--audio-only::after, .publish-ui__status-indicator-dot--video-only::after { display: block; }
| <div class="flex--center"> | ||
| <Button | ||
| title="No Source" | ||
| class={`publishSourceButton ${context.nothingActive() ? "active" : ""}`} | ||
| class={`publish-ui__source-button flex--center publish-ui__source-button--no-source ${context.nothingActive() ? "publish-ui__source-button--no-source-active" : ""}`} | ||
| onClick={onClick} |
There was a problem hiding this comment.
Use the shared wrapper class to keep spacing consistent.
Other source buttons use publish-ui__source-button-wrapper; without it, the “No Source” button may lose spacing/margins defined in source-button.css, leading to misalignment.
✅ Suggested fix
- <div class="flex--center">
+ <div class="publish-ui__source-button-wrapper flex--center">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div class="flex--center"> | |
| <Button | |
| title="No Source" | |
| class={`publishSourceButton ${context.nothingActive() ? "active" : ""}`} | |
| class={`publish-ui__source-button flex--center publish-ui__source-button--no-source ${context.nothingActive() ? "publish-ui__source-button--no-source-active" : ""}`} | |
| onClick={onClick} | |
| <div class="publish-ui__source-button-wrapper flex--center"> | |
| <Button | |
| title="No Source" | |
| class={`publish-ui__source-button flex--center publish-ui__source-button--no-source ${context.nothingActive() ? "publish-ui__source-button--no-source-active" : ""}`} | |
| onClick={onClick} |
🤖 Prompt for AI Agents
In `@js/hang-ui/src/publish/components/NothingSourceButton.tsx` around lines 14 -
18, The "No Source" button in NothingSourceButton is missing the shared wrapper
class used by other source buttons; update the component so the Button is
wrapped (or the outer div) uses the publish-ui__source-button-wrapper class to
ensure spacing/margins from source-button.css are applied (locate the JSX in the
NothingSourceButton component where the Button with class
publish-ui__source-button--no-source is rendered and add the
publish-ui__source-button-wrapper wrapper class to match other source buttons).
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Seems fine to me, although I don't understand the muted changes. Just make sure you've tested all of the permutations.
Thank you for the review, @kixelated. |
| <Button | ||
| title="Camera" | ||
| class={`publishSourceButton ${context.cameraActive() ? "active" : ""}`} | ||
| class={`publish-ui__source-button flex--center ${context.cameraActive() ? "publish-ui__source-button--active" : ""}`} |
There was a problem hiding this comment.
this is really cool! we should file an issue to re-factor all of the UI components to use this format.
There was a problem hiding this comment.
Glad you like it! I agree we should use this format.
jdreetz
left a comment
jdreetz
left a comment
There was a problem hiding this comment.
This looks great! Love the refactoring of the styles too! Haven't tested but I'm good with merging if it works for you.
3 participants
Restyled the publish UI and cleaned up the code.
Here are some image examples: