#279 Rebuild components/rating/rating-modal.tsx with shadcn Dialog pr… - #289
Conversation
…n Dialog primitives
|
@Chigybillionz is attempting to deploy a commit to the Threadflow Team on Vercel. A member of the Team first needs to authorize it. |
|
@Chigybillionz Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughThe rating modal now uses dialog primitives and shared form components, with updated success and error rendering. The rating stars component now relies on Tailwind class names instead of inline styles. ChangesRating UI refresh
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/rating/rating-stars.tsx`:
- Around line 26-27: Honor the disabled state in RatingStars so it is not
focusable and does not react to hover previews when disabled. Update the
focus/ARIA logic around tabIndex and role, and gate the hover handlers and
preview state updates in the RatingStars component so disabled behaves like
displayOnly for interaction while preserving the existing click/keyboard
blocking.
- Around line 46-53: The read-only branch in rating-stars.tsx is forcing every
star to the muted style, which hides the stored rating in displayOnly mode.
Update the className logic in the RatingStars rendering so the displayOnly path
still compares the current star against value and applies the filled/yellow
class for rated stars, while keeping the non-interactive styling separate; use
the existing displayOnly, hovered, value, and star symbols to preserve the
component’s read-only behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d2d72b0-eee3-4a2a-be09-9a8178cad8a2
📒 Files selected for processing (2)
components/rating/rating-modal.tsxcomponents/rating/rating-stars.tsx
| tabIndex={displayOnly ? -1 : 0} | ||
| role={displayOnly ? 'img' : 'slider'} | ||
| role={displayOnly ? "img" : "slider"} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Honor disabled in focus and hover handling.
disabled still leaves the slider focusable and still updates the hover preview, so the control looks interactive even though click/keyboard changes are blocked.
Suggested fix
- tabIndex={displayOnly ? -1 : 0}
+ tabIndex={displayOnly || disabled ? -1 : 0}
role={displayOnly ? "img" : "slider"}
+ aria-disabled={disabled || undefined}
aria-valuenow={value}
@@
- ? "flex items-center gap-1 outline-none"
- : "flex items-center gap-1 cursor-pointer outline-none"
+ ? "flex items-center gap-1 outline-none"
+ : `flex items-center gap-1 outline-none ${disabled ? "cursor-default" : "cursor-pointer"}`
@@
- onMouseEnter={() => !displayOnly && setHovered(star)}
- onMouseLeave={() => !displayOnly && setHovered(null)}
+ onMouseEnter={() => !displayOnly && !disabled && setHovered(star)}
+ onMouseLeave={() => !displayOnly && !disabled && setHovered(null)}Also applies to: 32-35, 41-45
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/rating/rating-stars.tsx` around lines 26 - 27, Honor the disabled
state in RatingStars so it is not focusable and does not react to hover previews
when disabled. Update the focus/ARIA logic around tabIndex and role, and gate
the hover handlers and preview state updates in the RatingStars component so
disabled behaves like displayOnly for interaction while preserving the existing
click/keyboard blocking.
| className={ | ||
| displayOnly | ||
| ? "select-none text-2xl text-muted-foreground" | ||
| : "select-none text-2xl transition-colors duration-200 " + | ||
| ((hovered ?? value) >= star | ||
| ? "text-yellow-400" | ||
| : "text-muted-foreground") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve filled stars in displayOnly mode.
This branch now renders every read-only star with the muted class, so a stored rating like 4 no longer shows four filled stars. That breaks the component’s read-only behavior.
Suggested fix
- className={
- displayOnly
- ? "select-none text-2xl text-muted-foreground"
- : "select-none text-2xl transition-colors duration-200 " +
- ((hovered ?? value) >= star
- ? "text-yellow-400"
- : "text-muted-foreground")
- }
+ className={
+ "select-none text-2xl " +
+ (!displayOnly ? "transition-colors duration-200 " : "") +
+ ((displayOnly ? value : hovered ?? value) >= star
+ ? "text-yellow-400"
+ : "text-muted-foreground")
+ }📝 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.
| className={ | |
| displayOnly | |
| ? "select-none text-2xl text-muted-foreground" | |
| : "select-none text-2xl transition-colors duration-200 " + | |
| ((hovered ?? value) >= star | |
| ? "text-yellow-400" | |
| : "text-muted-foreground") | |
| } | |
| className={ | |
| "select-none text-2xl " + | |
| (!displayOnly ? "transition-colors duration-200 " : "") + | |
| ((displayOnly ? value : hovered ?? value) >= star | |
| ? "text-yellow-400" | |
| : "text-muted-foreground") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/rating/rating-stars.tsx` around lines 46 - 53, The read-only
branch in rating-stars.tsx is forcing every star to the muted style, which hides
the stored rating in displayOnly mode. Update the className logic in the
RatingStars rendering so the displayOnly path still compares the current star
against value and applies the filled/yellow class for rated stars, while keeping
the non-interactive styling separate; use the existing displayOnly, hovered,
value, and star symbols to preserve the component’s read-only behavior.
Benjtalkshow
left a comment
Benjtalkshow
left a comment
There was a problem hiding this comment.
Nice cleanup, all acceptance criteria from #279 are met and tsc + lint pass
locally. Two small polishes before merge: replace the three Bounty:
/ Contributor: / Current Reputation:
labels with the muted-label + value pattern other dialogs in the
bounty surface use, and drop the unnecessary explicit React.ChangeEvent
annotation on the Textarea onChange. Also please attach a screenshot of the
rating modal (both the default form and the success state) so I can verify
the visual result before merging.
Alright.. I'll make the correction and give you a feedback on the outcome today! |
good day, you can now make the review please |
2 participants
Rebuild rating-modal.tsx with shadcn Dialog primitives
closes #279
Summary
components/rating/rating-modal.tsx was the only dialog in the repo using raw HTML and inline styles instead of the project's design system. This PR rebuilds it on the shadcn primitives that every other dialog uses, with semantic design tokens throughout. Props and behavior are unchanged, so the existing caller in components/bounty/bounty-sidebar.tsx keeps working without modification.
What changed
components/rating/rating-modal.tsx rebuilt with:
Dialog + DialogContent + DialogHeader + DialogTitle + DialogDescription + DialogFooter from components/ui/dialog
Button from components/ui/button — variant="ghost" for Cancel/Close, default variant for Submit
Textarea from components/ui/textarea
All style={{...}} replaced with Tailwind utility classes
Error text now uses the text-destructive token instead of color: 'red'
Open/close wired through Dialog's onOpenChange, preserving the existing onClose contract
components/rating/rating-stars.tsx updated to drop inline styles in favor of Tailwind classes / semantic tokens (text-muted-foreground, text-yellow-400) as part of removing all inline styling from this surface.
Props & behavior preserved
Props unchanged: contributor, bounty, onSubmit, onClose — no caller changes required (components/bounty/bounty-sidebar.tsx still works as-is).
Submit validates the 1–5 rating, calls onSubmit, shows a loading state, and renders the success state on resolve.
Error state renders correctly with the destructive token; Cancel/Close behave identically to before.
Acceptance criteria, raw , raw <textarea>, or style={{...}} in the file
No raw
Modal matches the visual treatment of other AlertDialogs in the bounty surface
Submit and Cancel buttons behave identically to before
Success and error states render correctly
pnpm tsc --noEmit passes (no new errors from these files)
pnpm lint passes (0 errors)
Testing
pnpm tsc --noEmit — clean for the rating components (the only remaining error is a pre-existing, unrelated @jest/globals types issue in components/reputation/tests/my-claims.test.ts).
pnpm lint — 0 errors (1 pre-existing unused-var warning in lib/server-graphql.ts, unrelated to this change).
npx eslint components/rating/rating-modal.tsx components/rating/rating-stars.tsx — exit 0.
Files
components/rating/rating-modal.tsx
components/rating/rating-stars.tsx
please kindly review and if there is any update regarding the task, please do let know!
Summary by CodeRabbit
New Features
Bug Fixes
Pushed the requested polish:
Screenshots attached below for both the default form and success state. initial
success