feat: add a copy button to the vote modal for a proposal link #86
Conversation
WalkthroughThe Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #86 +/- ##
==========================================
+ Coverage 58.85% 58.91% +0.06%
==========================================
Files 145 145
Lines 14040 14071 +31
==========================================
+ Hits 8263 8290 +27
- Misses 5777 5781 +4 ☔ View full report in Codecov by Sentry. |
![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
🧹 Outside diff range and nitpick comments (2)
components/groups/modals/voteDetailsModal.tsx (2)
424-429: Consider enhancing the URL constructionThe URL construction looks good, but consider these improvements:
- Add error handling for clipboard operations
- Consider using environment variables for the base URL instead of window.location.origin
const copyProposalLink = () => { - const url = `${window.location.origin}/groups?policyAddress=${proposal?.group_policy_address}&proposalId=${proposal?.id}`; - navigator.clipboard.writeText(url); - setCopied(true); - setTimeout(() => setCopied(false), 2000); + try { + const baseUrl = process.env.NEXT_PUBLIC_APP_URL || window.location.origin; + const url = `${baseUrl}/groups?policyAddress=${proposal?.group_policy_address}&proposalId=${proposal?.id}`; + navigator.clipboard.writeText(url).then(() => { + setCopied(true); + setTimeout(() => setCopied(false), 2000); + }).catch(error => { + console.error('Failed to copy:', error); + }); + } catch (error) { + console.error('Failed to copy:', error); + } };
517-531: Consider accessibility improvements for the share buttonWhile the implementation is good, consider these accessibility enhancements:
<button onClick={copyProposalLink} + aria-label={copied ? "Copied to clipboard" : "Share proposal"} + title="Copy proposal link" className="flex flex-row items-center gap-2 hover:bg-[#FFFFFFCC] dark:hover:bg-[#FFFFFF0F] p-2 rounded-full transition-colors duration-200" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/groups/modals/voteDetailsModal.tsx(5 hunks)
🔇 Additional comments (4)
components/groups/modals/voteDetailsModal.tsx (4)
25-25: LGTM: Clean state management setup
The new imports and state management for the copy functionality are well-structured.
Also applies to: 422-423
483-490: LGTM: Well-structured summary section
The conditional rendering and styling of the summary section is clean and follows best practices.
493-493: Verify the impact of reduced message height
The maximum height of messages section has been reduced from 20rem to 17rem. Ensure this doesn't cut off important content.
✅ Verification successful
Height reduction appears safe based on content patterns
Based on the codebase analysis:
- The messages section primarily displays message types and basic metadata
- Similar scrollable containers in the codebase use varying heights:
- Members list: 22.5rem
- Message type dropdown: 12.5rem
- Group details: 9.53rem
- The message content is compact, showing type and basic fields in a card format
- The container has overflow-y-auto, ensuring scrollability for edge cases
The reduction from 20rem to 17rem maintains a reasonable viewing area while keeping the modal compact, and scrolling is available when needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for message content patterns to assess typical length
ast-grep --pattern 'proposal?.messages?.map((message: any, index: number) => { $$$'
Length of output: 85
Script:
#!/bin/bash
# Search for message mapping patterns with different syntax
rg -U "messages.*map.*=>" --type ts --type tsx -A 5
# Search for the container div with height specification
rg "max-h-\[.*rem\].*overflow-y-auto" --type ts --type tsx -A 2 -B 2
Length of output: 180
Script:
#!/bin/bash
# Search for message mapping patterns with different syntax, without file type restrictions
rg -U "messages.*map.*=>" -A 5
# Search for the container div with height specification
rg "max-h-\[.*rem\].*overflow-y-auto" -A 2 -B 2
Length of output: 9361
433-433: Verify modal's maximum height constraint
The modal's max-height is set to 80vh which might be too restrictive on smaller screens.
✅ Verification successful
80vh max-height is consistent with other modals and provides good UX
Looking at the codebase, the 80vh constraint in voteDetailsModal is appropriate because:
- Other similar complex modals use 70vh (multiMfxMintModal and multiMfxBurnModal)
- The modal has overflow-y-auto to ensure content is scrollable
- The constraint prevents the modal from taking up the entire viewport while still providing ample space for content
- The implementation includes responsive design with flex-col/flex-row layout
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar modals in the codebase to compare height constraints
rg -l "modal-box.*max-h-\[.*vh\]" --type tsx
Length of output: 74
Script:
#!/bin/bash
# Search for similar modals in the codebase with height constraints, accounting for different file extensions
rg -l "modal-box.*max-h-\[.*vh\]" --type ts --type jsx --type tsx --type js
Length of output: 105
Script:
#!/bin/bash
# Search for all modal-box usages to understand common patterns
rg "modal-box" --type ts --type jsx --type tsx --type js -A 2
Length of output: 89
Script:
#!/bin/bash
# Find all modal-related files to analyze modal implementations
fd -e tsx -e jsx -e ts -e js . | rg -i "modal"
Length of output: 1315
Script:
#!/bin/bash
# Search for modal-box class usage with simpler ripgrep command
rg "modal-box" -A 2
Length of output: 8498
Script:
#!/bin/bash
# Search specifically for max-height constraints in modal components
rg "max-h-\[.*vh\]" components/
Length of output: 1428
Script:
#!/bin/bash
# Look at the base modal component implementation
cat components/react/modal.tsx
Length of output: 8219
| <div className="flex-row gap-2 items-center flex md:hidden mb-2"> | ||
| <button | ||
| onClick={copyProposalLink} | ||
| className="flex flex-row items-center gap-2 hover:bg-[#FFFFFFCC] dark:hover:bg-[#FFFFFF0F] p-2 rounded-full transition-colors duration-200" | ||
| > | ||
| {copied ? ( | ||
| <CheckIcon className="w-4 h-4 text-green-500" /> | ||
| ) : ( | ||
| <ShareIcon className="w-4 h-4" /> | ||
| )} | ||
| <p className="text-sm font-light text-gray-500 dark:text-gray-400"> | ||
| {copied ? 'Copied!' : 'Share this proposal'} | ||
| </p> | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
DRY: Consider extracting share button component
The share button implementation is duplicated between desktop and mobile views. Consider extracting it into a reusable component.
+ const ShareButton = ({ onClick, copied }) => (
+ <button
+ onClick={onClick}
+ aria-label={copied ? "Copied to clipboard" : "Share proposal"}
+ title="Copy proposal link"
+ className="flex flex-row items-center gap-2 hover:bg-[#FFFFFFCC] dark:hover:bg-[#FFFFFF0F] p-2 rounded-full transition-colors duration-200"
+ >
+ {copied ? (
+ <CheckIcon className="w-4 h-4 text-green-500" />
+ ) : (
+ <ShareIcon className="w-4 h-4" />
+ )}
+ <p className="text-sm font-light text-gray-500 dark:text-gray-400">
+ {copied ? 'Copied!' : 'Share this proposal'}
+ </p>
+ </button>
+ );
// Then use it in both places:
- <button onClick={copyProposalLink} className="...">
+ <ShareButton onClick={copyProposalLink} copied={copied} />Committable suggestion skipped: line range outside the PR's diff.
fixes #26
Summary by CodeRabbit
New Features
Improvements