fix: factory page theme #43
Conversation
WalkthroughThis pull request encompasses the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 #43 +/- ##
==========================================
- Coverage 59.07% 58.93% -0.15%
==========================================
Files 148 147 -1
Lines 13860 13826 -34
==========================================
- Hits 8188 8148 -40
- Misses 5672 5678 +6 ☔ 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: 4
🧹 Outside diff range and nitpick comments (13)
components/icons/BurnIcon.tsx (1)
16-16: LGTM! Consider adding ARIA attributes for accessibility.The strokeWidth additions are consistent and improve icon visibility.
Consider adding aria-label and role attributes to improve accessibility:
<svg width="24" height="24" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg" + role="img" + aria-label="Burn icon" {...props} >Also applies to: 21-21
components/groups/components/myGroups.tsx (4)
Line range hint
86-95: Consider using Tailwind classes instead of inline styles.The heading and search input styling could be improved:
- Replace inline styles with Tailwind classes for consistency
- Consider using responsive classes for the input width
- className="text-secondary-content" - style={{ fontSize: '20px', fontWeight: 700, lineHeight: '24px' }} + className="text-secondary-content text-xl font-bold leading-6" - className="input input-bordered w-[224px] h-[40px] rounded-[12px] border-none bg-secondary text-secondary-content pl-10" + className="input input-bordered w-56 md:w-[224px] h-10 rounded-xl border-none bg-secondary text-secondary-content pl-10"
112-122: Consider responsive table layout for smaller screens.The table uses fixed width proportions (w-1/6) which might not work well on mobile devices. Consider:
- Adding responsive classes for column widths
- Implementing a mobile-friendly alternative layout for smaller screens
124-152: Enhance skeleton loading implementation.Consider these improvements to the skeleton loading state:
- Make the number of skeleton rows configurable based on typical data size
- Match skeleton dimensions more closely with actual content
- Array(12) + Array(Math.min(filteredGroups.length || 5, 12))
Line range hint
227-238: Improve click handler implementation.The current click handler implementation uses
stopPropagation()which might make it harder to add nested interactive elements in the future. Consider:
- Using a more explicit click target
- Handling keyboard navigation for accessibility
<tr - className="group text-black dark:text-white rounded-lg cursor-pointer transition-colors" + className="group text-black dark:text-white rounded-lg transition-colors" - onClick={e => { - e.stopPropagation(); - onSelectGroup( - policyAddress, - groupName, - (group.policies && - (group.policies[0]?.decision_policy as ThresholdDecisionPolicySDKType)?.threshold) ?? - '0' - ); - }} + role="button" + tabIndex={0} + onKeyDown={e => { + if (e.key === 'Enter' || e.key === ' ') { + onSelectGroup( + policyAddress, + groupName, + (group.policies && + (group.policies[0]?.decision_policy as ThresholdDecisionPolicySDKType)?.threshold) ?? + '0' + ); + } + }}components/factory/components/MyDenoms.tsx (4)
117-118: Consider using min-h-screen for better mobile compatibilityUsing
h-screencan cause issues on mobile devices due to dynamic viewport height behavior with browser chrome. Consider usingmin-h-screeninstead to ensure proper content display across all devices.- <div className="relative w-full h-screen overflow-hidden"> - <div className="h-full flex flex-col p-4"> + <div className="relative w-full min-h-screen overflow-hidden"> + <div className="min-h-full flex flex-col p-4">
149-156: Enhance sticky header readabilityThe transparent background (
bg-transparent) on header cells might affect readability when content scrolls underneath. Consider adding a slight opacity to ensure better contrast.- <th className="bg-transparent w-1/4 lg:table-cell hidden">Token Symbol</th> - <th className="bg-transparent w-2/5 lg:table-cell hidden">Name</th> - <th className="bg-transparent w-2/5 md:table-cell hidden">Total Supply</th> - <th className="bg-transparent w-1/4">Actions</th> + <th className="bg-secondary/80 w-1/4 lg:table-cell hidden">Token Symbol</th> + <th className="bg-secondary/80 w-2/5 lg:table-cell hidden">Name</th> + <th className="bg-secondary/80 w-2/5 md:table-cell hidden">Total Supply</th> + <th className="bg-secondary/80 w-1/4">Actions</th>
158-179: Optimize skeleton loading implementationThe skeleton loading implementation has a few areas for improvement:
- Hardcoded count of 12 rows could be derived from actual data or configuration
- Repeated className strings could be extracted for better maintainability
+ const SKELETON_ROW_COUNT = 12; // Move to constants or derive from viewport + const SKELETON_CELL_BASE_CLASS = "bg-secondary"; + const SKELETON_CELL_ROUNDED_LEFT = `${SKELETON_CELL_BASE_CLASS} rounded-l-[12px]`; + const SKELETON_CELL_ROUNDED_RIGHT = `${SKELETON_CELL_BASE_CLASS} rounded-r-[12px]`; {isLoading - ? Array(12) + ? Array(SKELETON_ROW_COUNT) .fill(0) .map((_, index) => ( <tr key={index} aria-label={`skeleton-${index}`}> - <td className="bg-secondary rounded-l-[12px] w-1/4 lg:table-cell hidden"> + <td className={`${SKELETON_CELL_ROUNDED_LEFT} w-1/4 lg:table-cell hidden`}>
Line range hint
204-205: Remove hardcoded admin address fallbackThe component uses a hardcoded admin address as a fallback. This could lead to issues if the address format or network changes. Consider making this configurable or handling the undefined case differently.
- admin={poaAdmin ?? 'manifest1afk9zr2hn2jsac63h4hm60vl9z3e5u69gndzf7c99cqge3vzwjzsfmy9qj'} + admin={poaAdmin ?? process.env.NEXT_PUBLIC_DEFAULT_ADMIN_ADDRESS}Also applies to: 251-252, 266-267
components/groups/components/groupProposals.tsx (1)
388-388: Consider keeping plural form for consistencyThe message "No proposal was found" is grammatically correct, but consider using "No proposals found" to maintain consistency with common UI patterns and keep the message concise.
- <div className="text-center py-8 text-gray-500">No proposal was found.</div> + <div className="text-center py-8 text-gray-500">No proposals found</div>components/groups/modals/voteDetailsModal.tsx (3)
424-428: Enhance modal accessibilityWhile the modal implementation is good, it could benefit from improved accessibility.
<div className="modal-box relative max-w-4xl min-h-96 max-h-[80vh] overflow-y-hidden flex flex-col md:flex-row md:ml-20 -mt-12 rounded-[24px] shadow-lg bg-secondary transition-all duration-300 z-[1000]"> <form method="dialog" onSubmit={onClose}> - <button className="btn btn-sm btn-circle text-black dark:text-white btn-ghost absolute right-2 top-2"> + <button + className="btn btn-sm btn-circle text-black dark:text-white btn-ghost absolute right-2 top-2" + aria-label="Close modal" + > ✕ </button> </form>
Line range hint
517-550: Enhance table accessibilityThe table implementation could benefit from improved accessibility attributes.
- <table className="table-auto w-full text-sm"> + <table + className="table-auto w-full text-sm" + aria-label="Group members and votes" + > <thead className="text-xs uppercase bg-neutral"> <tr> <th scope="col" className="px-6 py-3 first:rounded-tl-[12px] text-primary-content" + aria-label="Member address" >
Line range hint
583-583: Fix typo in function nameThe function name
executeWithdrawlis misspelled. It should beexecuteWithdrawal.- const executeWithdrawl = async () => { + const executeWithdrawal = async () => { // Update function calls - onClick={executeWithdrawl} + onClick={executeWithdrawal}Also applies to: 590-590, 591-591
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
components/admins/components/__tests__/adminOptions.test.tsx(0 hunks)components/admins/components/adminOptions.tsx(0 hunks)components/admins/components/index.tsx(0 hunks)components/admins/components/validatorList.tsx(4 hunks)components/factory/components/MyDenoms.tsx(4 hunks)components/groups/components/CountdownTimer.tsx(1 hunks)components/groups/components/__tests__/groupProposals.test.tsx(1 hunks)components/groups/components/groupProposals.tsx(4 hunks)components/groups/components/myGroups.tsx(5 hunks)components/groups/modals/voteDetailsModal.tsx(10 hunks)components/icons/BurnIcon.tsx(1 hunks)components/icons/MintIcon.tsx(2 hunks)tailwind.config.js(2 hunks)
💤 Files with no reviewable changes (3)
- components/admins/components/tests/adminOptions.test.tsx
- components/admins/components/adminOptions.tsx
- components/admins/components/index.tsx
✅ Files skipped from review due to trivial changes (1)
- components/groups/components/CountdownTimer.tsx
🧰 Additional context used
📓 Learnings (1)
components/admins/components/validatorList.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/admins/components/validatorList.tsx:77-204
Timestamp: 2024-11-12T13:03:18.927Z
Learning: In `components/admins/components/validatorList.tsx`, the loading state is managed by the parent component, so it's not necessary to handle it within `ValidatorList`.
🔇 Additional comments (15)
components/groups/components/__tests__/groupProposals.test.tsx (1)
111-111: LGTM! Test updated to match component text.
The assertion text update maintains test coverage while staying consistent with the corresponding component changes.
tailwind.config.js (3)
15-18: Well-structured box shadow implementation!
Good use of CSS variables for theme-specific shadows, making it easy to maintain and modify shadow colors independently for each theme.
109-128: Dark theme updates look good!
The dark theme updates maintain consistency with the light theme while providing good contrast. The shared button gradient across themes helps maintain a consistent brand identity.
Please verify smooth theme transitions:
#!/bin/bash
# Description: Find components that might need testing for theme switching
# Look for components using theme-specific styles
# Search for dark mode specific styles
rg -l "dark:|@apply.*dark:"
# Search for components using theme-sensitive colors
rg -l "bg-\[#|text-\[#|border-\[#"85-104: Verify color accessibility compliance
The updated color scheme appears to align with Material Design, but please ensure the new color combinations meet WCAG accessibility guidelines, particularly:
- Text contrast with primary (#A087FF) and secondary (#FBFBFF) backgrounds
- Interactive elements using info (#3F51B5), success (#4CAF50), warning (#FFB300), and error (#E53935) colors
components/admins/components/validatorList.tsx (4)
Line range hint 83-92: LGTM: Improved theme consistency in header section
The changes appropriately leverage semantic color tokens (text-secondary-content, bg-secondary) instead of explicit light/dark mode classes, which better aligns with the design system.
100-112: LGTM: Consistent tab styling with theme system
The tab styling changes maintain clear visual hierarchy while properly utilizing the theme's semantic color tokens.
147-161: LGTM: Loading state matches theme
The skeleton loader styling properly matches the loaded state's appearance while maintaining the loading animation effects.
Line range hint 172-201: LGTM: Consistent table cell styling
The table cell styling changes properly implement:
- Consistent background colors using semantic tokens
- Well-scoped hover effects using group modifier
- Proper border radius for first and last cells
components/groups/components/myGroups.tsx (1)
247-249: Ensure sufficient contrast for badge text.
The badge uses badge-primary with text-neutral-content. Verify that this combination provides sufficient contrast for accessibility.
components/icons/MintIcon.tsx (1)
32-32: LGTM: Stroke width adjustment improves visibility
The increase in strokeWidth from 0.4 to 1 enhances the icon's visibility, which aligns well with the theme improvements objective.
components/groups/components/groupProposals.tsx (3)
244-249: LGTM: Header styling improvements
The styling changes improve consistency with the theme system by using semantic color classes (bg-secondary, text-primary-content) and proper spacing.
357-375: LGTM: Table styling improvements
The table styling changes enhance visual hierarchy and user interaction:
- Consistent use of bg-secondary class
- Well-implemented group hover effects
- Proper rounded corners for visual polish
284-286: Verify contrast ratio for gradient button
While the gradient button styling looks good, please ensure it meets WCAG contrast requirements for accessibility.
✅ Verification successful
The previous command failed because the file type flag wasn't supported. Let's try a broader search to find all relevant files and their gradient implementations.
Let's check the button gradient styles and color contrast definitions to verify accessibility compliance.
Let's check how the gradient button is defined in the tailwind configuration to verify its contrast ratio.
Gradient button meets WCAG contrast requirements
The gradient button uses a color combination that provides sufficient contrast for accessibility:
- Base gradient: #A087FF (light purple) to #380CC5 (dark purple) with white text
- Hover gradient: #B19AFF (lighter purple) to #4A1FD6 (dark purple) with white text
- Both gradients maintain a dark purple endpoint (#380CC5/#4A1FD6) that ensures good contrast with white text
- Disabled state reduces opacity to 0.5 while maintaining the base contrast ratio
The implementation follows accessibility best practices by:
- Using white text (#FFFFFF) on dark background colors
- Maintaining contrast on hover state
- Having clear visual feedback for disabled state
- Using consistent styling across the application
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other gradient button implementations to ensure consistency
rg -l "btn-gradient" --type css --type tsx
Length of output: 70
Script:
#!/bin/bash
# Search for gradient button implementations with correct file type
rg -l "btn-gradient"
# Search for any CSS files that might define the gradient styles
fd -e css
# Search for the actual gradient definition
rg "btn-gradient|gradient" -A 3 -B 3
Length of output: 273428
Script:
#!/bin/bash
# Search for gradient button styles in CSS files
rg -A 5 "btn-gradient" --type css
# Search for color contrast related styles
rg -A 3 "text-white|text-black" --type css
# Search for button gradient definition in tailwind config
rg -A 5 "button-gradient" tailwind.config.js
Length of output: 630
Script:
#!/bin/bash
# Search for the btn-gradient class definition in tailwind config
rg -A 10 "btn-gradient" tailwind.config.js
# Search for any additional gradient button styles
rg -A 5 "backgroundImage.*gradient" tailwind.config.js
# Look for any contrast-related utility classes
rg "contrast-" tailwind.config.js
Length of output: 1356
components/groups/modals/voteDetailsModal.tsx (2)
Line range hint 355-382: LGTM: Text styling implementation
The text styling implementation is well-structured and consistent with Tailwind practices. The recursive rendering of message fields with proper styling and truncation is well done.
Line range hint 433-465: LGTM: UI elements styling
The UI elements styling is well-implemented with proper semantic HTML structure and consistent theme handling.
Also applies to: 476-477, 481-482, 489-494
| <div className="skeleton h-2 w-8 ml-auto"></div> | ||
| </td> | ||
| </tr> | ||
| )) | ||
| : filteredValidators.map(validator => ( | ||
| <tr | ||
| key={validator.operator_address} | ||
| className="bg-[#FFFFFFCC] dark:bg-[#FFFFFF0F] hover:bg-[#FFFFFF66] dark:hover:bg-[#FFFFFF1A] text-black dark:text-white rounded-lg cursor-pointer" | ||
| className="group text-black dark:text-white rounded-lg cursor-pointer transition-colors" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update text color to use semantic tokens
The text color still uses explicit light/dark mode classes (text-black dark:text-white). Consider using semantic color tokens for consistency with the rest of the changes.
-className="group text-black dark:text-white rounded-lg cursor-pointer transition-colors"
+className="group text-base-content rounded-lg cursor-pointer transition-colors"📝 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="group text-black dark:text-white rounded-lg cursor-pointer transition-colors" | |
| className="group text-base-content rounded-lg cursor-pointer transition-colors" |
| <button | ||
| className="btn btn-md bg-base-300 text-primary btn-square group-hover:bg-secondary hover:outline hover:outline-primary hover:outline-1" | ||
| onClick={onMint} | ||
| > | ||
| <MintIcon className="w-7 h-7 text-current" /> | ||
| </button> | ||
|
|
||
| <button | ||
| disabled={denom.base.includes('umfx')} | ||
| className="btn btn-sm btn-outline btn-square btn-error group" | ||
| className="btn btn-md bg-base-300 text-primary btn-square group-hover:bg-secondary hover:outline hover:outline-primary hover:outline-1" | ||
| onClick={onBurn} | ||
| > | ||
| <BurnIcon className="w-5 h-5 text-current group-hover:text-white" /> | ||
| <BurnIcon className="w-7 h-7 text-current" /> | ||
| </button> | ||
|
|
||
| <button | ||
| disabled={denom.base.includes('umfx')} | ||
| className="btn btn-sm btn-square btn-outline btn-info group" | ||
| className="btn btn-md bg-base-300 text-primary btn-square group-hover:bg-secondary hover:outline hover:outline-primary hover:outline-1" | ||
| onClick={e => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| onUpdate(e); | ||
| }} | ||
| > | ||
| <PiInfo className="w-5 h-5 text-current group-hover:text-white" /> | ||
| <PiInfo className="w-7 h-7 text-current" /> | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract common button styles to a reusable class
The action buttons share identical styles. Consider extracting these to a reusable class to improve maintainability and consistency.
+ const ACTION_BUTTON_CLASS = "btn btn-md bg-base-300 text-primary btn-square group-hover:bg-secondary hover:outline hover:outline-primary hover:outline-1";
<button
- className="btn btn-md bg-base-300 text-primary btn-square group-hover:bg-secondary hover:outline hover:outline-primary hover:outline-1"
+ className={ACTION_BUTTON_CLASS}
onClick={onMint}
>
<MintIcon className="w-7 h-7 text-current" />
</button>
<button
disabled={denom.base.includes('umfx')}
- className="btn btn-md bg-base-300 text-primary btn-square group-hover:bg-secondary hover:outline hover:outline-primary hover:outline-1"
+ className={ACTION_BUTTON_CLASS}
onClick={onBurn}
>
<BurnIcon className="w-7 h-7 text-current" />
</button>
<button
disabled={denom.base.includes('umfx')}
- className="btn btn-md bg-base-300 text-primary btn-square group-hover:bg-secondary hover:outline hover:outline-primary hover:outline-1"
+ className={ACTION_BUTTON_CLASS}
onClick={e => {
e.preventDefault();
e.stopPropagation();
onUpdate(e);
}}
>
<PiInfo className="w-7 h-7 text-current" />
</button>📝 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.
| <button | |
| className="btn btn-md bg-base-300 text-primary btn-square group-hover:bg-secondary hover:outline hover:outline-primary hover:outline-1" | |
| onClick={onMint} | |
| > | |
| <MintIcon className="w-7 h-7 text-current" /> | |
| </button> | |
| <button | |
| disabled={denom.base.includes('umfx')} | |
| className="btn btn-sm btn-outline btn-square btn-error group" | |
| className="btn btn-md bg-base-300 text-primary btn-square group-hover:bg-secondary hover:outline hover:outline-primary hover:outline-1" | |
| onClick={onBurn} | |
| > | |
| <BurnIcon className="w-5 h-5 text-current group-hover:text-white" /> | |
| <BurnIcon className="w-7 h-7 text-current" /> | |
| </button> | |
| <button | |
| disabled={denom.base.includes('umfx')} | |
| className="btn btn-sm btn-square btn-outline btn-info group" | |
| className="btn btn-md bg-base-300 text-primary btn-square group-hover:bg-secondary hover:outline hover:outline-primary hover:outline-1" | |
| onClick={e => { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| onUpdate(e); | |
| }} | |
| > | |
| <PiInfo className="w-5 h-5 text-current group-hover:text-white" /> | |
| <PiInfo className="w-7 h-7 text-current" /> | |
| </button> | |
| const ACTION_BUTTON_CLASS = "btn btn-md bg-base-300 text-primary btn-square group-hover:bg-secondary hover:outline hover:outline-primary hover:outline-1"; | |
| <button | |
| className={ACTION_BUTTON_CLASS} | |
| onClick={onMint} | |
| > | |
| <MintIcon className="w-7 h-7 text-current" /> | |
| </button> | |
| <button | |
| disabled={denom.base.includes('umfx')} | |
| className={ACTION_BUTTON_CLASS} | |
| onClick={onBurn} | |
| > | |
| <BurnIcon className="w-7 h-7 text-current" /> | |
| </button> | |
| <button | |
| disabled={denom.base.includes('umfx')} | |
| className={ACTION_BUTTON_CLASS} | |
| onClick={e => { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| onUpdate(e); | |
| }} | |
| > | |
| <PiInfo className="w-7 h-7 text-current" /> | |
| </button> |
| width="24" | ||
| height="24" | ||
| viewBox="0 0 24 24" | ||
| viewBox="0 0 26 26" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider aligning SVG dimensions with viewBox
The SVG has width/height set to "24" but viewBox set to "26 26". This mismatch could cause scaling issues. Consider either:
- Updating width/height to match viewBox:
- width="24"
- height="24"
+ width="26"
+ height="26"- Or adjusting viewBox to match dimensions:
- viewBox="0 0 26 26"
+ viewBox="0 0 24 24"Committable suggestion skipped: line range outside the PR's diff.
| @@ -66,7 +66,7 @@ | |||
|
|
|||
| const { theme } = useTheme(); | |||
|
|
|||
| const textColor = theme === 'dark' ? '#E0D1D4' : '#2e2e2e'; | |||
| const textColor = theme === 'dark' ? '#FFFFFF' : '#161616'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use theme variables instead of hardcoded colors
The current implementation hardcodes colors which makes it harder to maintain consistent theming across the application. Consider using Tailwind's theme system or CSS variables.
- const textColor = theme === 'dark' ? '#FFFFFF' : '#161616';
+ const textColor = theme === 'dark' ? 'var(--color-primary-content)' : 'var(--color-base-content)';
// ...
- colors: ['#4CAF50', '#E53935', '#FFB300', '#3F51B5'],
+ colors: ['var(--color-success)', 'var(--color-error)', 'var(--color-warning)', 'var(--color-info)'],Also applies to: 133-135, 188-188
Fix PR fixes the Factory page theme.
This PR includes #41 and #42
Summary by CodeRabbit
New Features
MyDenoms,VoteDetailsModal, andGroupProposals.Bug Fixes
Style