feat: use Slider for Conviction settings #1629
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (10)
packages/extension-polkagate/src/themes/light.ts (2)
Line range hint
11-27: Consider renaming 'backgroundFL' to be more descriptive.While the voting-related palette properties (aye, nay, approval, support) are self-explanatory, the purpose of 'backgroundFL' is not immediately clear from its name. Consider using a more descriptive name that indicates its specific use case.
The rest of the interface declarations look good and properly type the new palette properties.
Line range hint
29-44: LGTM! Consider adding color documentation.The color choices are well-thought-out:
- Good contrast between aye (#008080) and nay (#FF5722) colors
- Appropriate success light variant (#46890C)
- Consistent color scheme throughout
Consider adding a comment block documenting the purpose of each color, especially for the voting-related palette properties, to help maintain consistency in future theme updates.
packages/extension-polkagate/src/themes/dark.ts (2)
Line range hint
11-26: Fix type inconsistency in backgroundFL property.The
backgroundFLproperty has inconsistent types between thePaletteandPaletteOptionsinterfaces:
- In
Palette:backgroundFL?: Palette['primary']- In
PaletteOptions:backgroundFL?: Partial<TypeText>This mismatch could lead to type-related issues during theme customization.
Apply this fix:
interface Palette { aye: Palette['primary']; nay: Palette['primary']; label: Palette['primary']; approval: Palette['primary']; support: Palette['primary']; - backgroundFL?: Palette['primary']; + backgroundFL: TypeText; }
Theme properties need standardization and hardcoded colors should be migrated
Based on the verification results:
- The theme properties (
aye,nay,approval,support,backgroundFL) are consistently used in governance and staking-related components for their intended purposes.- However, there are numerous instances of hardcoded colors (especially
#BA2882and#99004F) across components that should be using theme properties instead.Key areas needing attention:
- Icons and SVG components frequently use hardcoded
#BA2882- Circle components consistently use hardcoded
#99004F- Arrow indicators and navigation elements use hardcoded stroke colors
Recommendations:
- Create semantic theme properties for commonly used colors (e.g.,
iconStroke,circleBackground)- Migrate hardcoded colors in components to use theme properties
- Document the color system in a style guide
🔗 Analysis chain
Line range hint
11-47: Consider documenting color usage guidelines.With the introduction of specialized color properties for voting-related UI elements (
aye,nay,approval, etc.), consider:
- Documenting the intended usage of each color property
- Creating a style guide for consistent application across components
- Adding color contrast verification in your CI pipeline
This will help maintain consistency as the voting UI evolves.
Let's check for consistent usage of these new color properties:
Let me analyze the usage of these color properties more deeply to verify consistency.
Let me modify the search to avoid the file type issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new color palette properties across components # Search for usage of new color properties echo "Checking usage of new color properties..." rg -l "palette\.(aye|nay|approval|support|backgroundFL)" # Look for potential direct color usage instead of theme properties echo "Checking for hardcoded colors that might need migration to theme..." rg -l "(#99004F|#BA2882|#BCE2DB|#BA82A4)"Length of output: 4491
Script:
#!/bin/bash # Check the actual implementation of components using these theme properties echo "=== Checking implementation of components using theme properties ===" rg -A 2 "palette\.(aye|nay|approval|support|backgroundFL)" --type tsx echo -e "\n=== Checking hardcoded color usage in components ===" # Excluding theme files and SVG assets to focus on component implementations rg "#(99004F|BA2882|BCE2DB|BA82A4)" --type tsx | grep -v "themes/" | grep -v "assets/"Length of output: 480
Script:
#!/bin/bash # Check the actual implementation of components using theme properties echo "=== Checking implementation of components using theme properties ===" rg -A 2 "palette\.(aye|nay|approval|support|backgroundFL)" echo -e "\n=== Checking hardcoded color usage in components ===" # Excluding theme files and SVG assets to focus on component implementations rg "#(99004F|BA2882|BCE2DB|BA82A4)" | grep -v "themes/" | grep -v "\.svg:"Length of output: 18989
packages/extension-polkagate/src/hooks/useConvictionOptions.ts (3)
10-13: LGTM! Consider organizing type imports.The type definitions and hook signature changes improve type safety and follow React best practices by using the
useTranslationhook instead of passing the translation function as a parameter.Consider grouping related type imports together:
-import type { AccountId } from '@polkadot/types/interfaces/runtime'; -import type { BN } from '@polkadot/util'; -import type { DropdownOption } from '../util/types'; +// External types +import type { AccountId } from '@polkadot/types/interfaces/runtime'; +import type { BN } from '@polkadot/util'; + +// Internal types +import type { DropdownOption } from '../util/types';Also applies to: 22-24
Line range hint
47-54: Consider improving storage handling.While the storage operations work, there are several potential improvements that could make the code more robust and maintainable:
- Abstract storage operations into a utility:
// storage.ts const STORAGE_KEY = 'Convictions'; interface ConvictionStorage { [genesisHash: string]: DropdownOption[]; } export const saveConvictions = async ( genesisHash: string, options: DropdownOption[] ): Promise<void> => { const result = await chrome.storage.local.get(STORAGE_KEY); const current = (result[STORAGE_KEY] || {}) as ConvictionStorage; await chrome.storage.local.set({ [STORAGE_KEY]: { ...current, [genesisHash]: options } }); }; export const loadConvictions = async ( genesisHash: string ): Promise<DropdownOption[] | undefined> => { const result = await chrome.storage.local.get(STORAGE_KEY); return (result[STORAGE_KEY] as ConvictionStorage)?.[genesisHash]; };
- Add error handling for storage operations:
try { await saveConvictions(genesisHash, options); } catch (error) { console.error('Failed to save convictions:', error); }Also applies to: 75-84
Line range hint
30-45: Consider extracting period calculation logic.The conviction options generation logic could be more maintainable with some refactoring:
const formatPeriod = ( blockTime: BN, voteLockingPeriod: BN, durationBn: BN ): string => { if (!voteLockingPeriod || voteLockingPeriod.lte(BN_ZERO)) { return ''; } const [, formattedTime] = calcBlockTime( blockTime, durationBn.mul(voteLockingPeriod), t ); return ` (${formattedTime})`; }; const getConvictionOptions = useCallback(( blockTime: BN, voteLockingPeriod: BN, genesisHash: string ) => { const options = [ { text: t('0.1x voting balance, no lockup period'), value: 0.1 }, ...CONVICTIONS.map(([value, duration, durationBn]) => ({ text: t('{{value}}x voting balance, locked for {{duration}}x duration{{period}}', { replace: { duration, period: formatPeriod(blockTime, voteLockingPeriod, durationBn), value } }), value })) ]; // ... rest of the function }, [t]);packages/extension-polkagate/src/fullscreen/governance/post/castVote/Cast.tsx (3)
Line range hint
54-77: Optimize lock sorting logic.The current implementation performs two separate sorts which is inefficient. Consider combining the sorting criteria into a single sort operation.
function getAlreadyLockedValue (allBalances: DeriveBalancesAll | undefined): BN | undefined { const sortedLocks = allBalances?.lockedBreakdown - // first sort by amount, so greatest value first - .sort((a, b) => - b.amount.cmp(a.amount) - ) - // then sort by the type of lock (we try to find relevant) - .sort((a, b): number => { + .sort((a, b): number => { + // First compare by lock type if (!a.id.eq(b.id)) { for (let i = 0; i < LOCKS_ORDERED.length; i++) { const lockName = LOCKS_ORDERED[i]; if (a.id.eq(lockName)) { return -1; } else if (b.id.eq(lockName)) { return 1; } } } - return 0; + // If lock types are equal or not in LOCKS_ORDERED, sort by amount + return b.amount.cmp(a.amount); }) .map(({ amount }) => amount); return sortedLocks?.[0] || allBalances?.lockedBalance; }
Line range hint
346-352: Add aria-labels for accessibility.The vote buttons use icons which should have aria-labels for better accessibility.
- <CheckIcon sx={{ color: STATUS_COLOR['Confirmed'], fontSize: '28px', stroke: STATUS_COLOR['Confirmed'], strokeWidth: 1.5 }} /> + <CheckIcon aria-label="Vote Aye" sx={{ color: STATUS_COLOR['Confirmed'], fontSize: '28px', stroke: STATUS_COLOR['Confirmed'], strokeWidth: 1.5 }} /> - <CloseIcon sx={{ color: 'warning.main', fontSize: '28px', stroke: theme.palette.warning.main, strokeWidth: 1.5 }} /> + <CloseIcon aria-label="Vote Nay" sx={{ color: 'warning.main', fontSize: '28px', stroke: theme.palette.warning.main, strokeWidth: 1.5 }} /> - <AbstainIcon sx={{ color: 'primary.light', fontSize: '28px' }} /> + <AbstainIcon aria-label="Abstain" sx={{ color: 'primary.light', fontSize: '28px' }} />
Line range hint
414-422: Consider using MUI's Grid spacing prop.Instead of using padding top (pt), consider using MUI's Grid spacing prop for consistent spacing throughout the component.
- <Grid alignItems='center' container item justifyContent='space-between' sx={{ height: '42px', pt: '25px' }}> + <Grid alignItems='center' container item justifyContent='space-between' spacing={3} sx={{ height: '42px' }}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/extension-polkagate/src/components/Convictions.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/governance/post/castVote/Cast.tsx(8 hunks)packages/extension-polkagate/src/hooks/useConvictionOptions.ts(3 hunks)packages/extension-polkagate/src/themes/dark.ts(1 hunks)packages/extension-polkagate/src/themes/light.ts(1 hunks)
🔇 Additional comments (5)
packages/extension-polkagate/src/themes/dark.ts (1)
45-45: LGTM! Good color choice for success states.
The addition of a light variant (#46890C) to the success color palette provides better visual hierarchy while maintaining good contrast ratios.
packages/extension-polkagate/src/hooks/useConvictionOptions.ts (2)
26-28: LGTM! Robust state management implementation.
The state management is well-implemented with:
- Proper typing for both current and saved options
- Fallback mechanism using saved options
- Safe access to API constants using optional chaining
22-22: Verify consistent usage of conviction options.
Let's ensure that all components using this hook handle the undefined return value correctly.
✅ Verification successful
Conviction options are handled correctly across the codebase
The verification shows that useConvictionOptions is used in three components, and all of them handle the undefined return value appropriately:
Convictions.tsx: Uses the options directly in a controlled componentdelegate/Delegate.tsx: Properly handles options alongside conviction statecastVote/Cast.tsx: Safely consumes options in the voting flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of useConvictionOptions to verify proper handling
rg -A 5 "useConvictionOptions" --type typescript
Length of output: 83
Script:
#!/bin/bash
# Search for usages of useConvictionOptions without file type restriction
rg -A 5 "useConvictionOptions"
Length of output: 6479
packages/extension-polkagate/src/fullscreen/governance/post/castVote/Cast.tsx (2)
91-92: LGTM! Good use of the useInfo hook.
Consolidating API, decimal, formatted, and token access into a single hook improves code organization and reusability.
Line range hint 149-157: Verify conviction initialization logic.
The conviction initialization logic has multiple places where DEFAULT_CONVICTION is set. Consider consolidating this logic to avoid potential race conditions.
|
|
||
| /* eslint-disable react/jsx-max-props-per-line */ | ||
| //@ts-nocheck |
There was a problem hiding this comment.
💡 Codebase verification
Remove @ts-nocheck directive and fix type issues.
The usage of @ts-nocheck is widespread across the codebase, particularly in the polkagate extension. However, this is not a good practice as it bypasses TypeScript's type checking system. Consider:
- Fixing the underlying type issues in Cast.tsx
- Using more specific type annotations where needed
- Using targeted @ts-ignore comments only for specific lines if absolutely necessary
🔗 Analysis chain
Remove @ts-nocheck directive.
Disabling TypeScript type checking for the entire file is not recommended as it can hide potential type-related issues. Consider fixing the underlying type issues instead.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other ts-ignore or ts-nocheck directives in the codebase
rg -l "@ts-(ignore|nocheck)" --type ts --type tsx
Length of output: 79
Script:
#!/bin/bash
# Check for other ts-ignore or ts-nocheck directives in the codebase, using ripgrep without file type filters
rg -l "@ts-(ignore|nocheck)"
Length of output: 25166
| </Typography> | ||
| <Slider | ||
| aria-label='Vote Convictions' | ||
| defaultValue={conviction || DEFAULT_CONVICTION} |
There was a problem hiding this comment.
Use 'value' instead of 'defaultValue' for the controlled Slider component.
Since conviction is a state managed via setConviction, the Slider should use the value prop instead of defaultValue to ensure it reflects the current state accurately.
Apply this diff:
- defaultValue={conviction || DEFAULT_CONVICTION}
+ value={conviction || DEFAULT_CONVICTION}📝 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.
| defaultValue={conviction || DEFAULT_CONVICTION} | |
| value={conviction || DEFAULT_CONVICTION} |
| const onChangeConviction = useCallback((_event: Event, newValue: number | number[], _activeThumb: number): void => { | ||
| setConviction(newValue as number); | ||
| }, [setConviction]); |
There was a problem hiding this comment.
Ensure 'newValue' is a number before casting in 'onChangeConviction'.
The newValue parameter in onChangeConviction can be a number or an array of numbers. Casting it directly to a number might lead to unexpected behavior if newValue is an array. It's safer to check if newValue is an array and handle it accordingly.
Apply this diff to fix the issue:
const onChangeConviction = useCallback((_event: Event, newValue: number | number[], _activeThumb: number): void => {
- setConviction(newValue as number);
+ const value = Array.isArray(newValue) ? newValue[0] : newValue;
+ setConviction(value as number);
}, [setConviction]);📝 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.
| const onChangeConviction = useCallback((_event: Event, newValue: number | number[], _activeThumb: number): void => { | |
| setConviction(newValue as number); | |
| }, [setConviction]); | |
| const onChangeConviction = useCallback((_event: Event, newValue: number | number[], _activeThumb: number): void => { | |
| const value = Array.isArray(newValue) ? newValue[0] : newValue; | |
| setConviction(value as number); | |
| }, [setConviction]); |
| useEffect(() => { | ||
| // Select all mark labels and apply styles based on slider value | ||
| const markLabels = document.querySelectorAll('.MuiSlider-markLabel'); | ||
|
|
||
| convictionOptions && markLabels.forEach((label, index) => { | ||
| const markValue = convictionOptions[index]?.value as number | undefined; | ||
|
|
||
| if (markValue && conviction && markValue > conviction) { | ||
| (label as HTMLElement).style.color = theme.palette.text.disabled; // Untraversed color | ||
| } else { | ||
| (label as HTMLElement).style.color = theme.palette.text.primary; | ||
| } | ||
| }); | ||
| }, [conviction, convictionOptions, theme]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid direct DOM manipulation; use Slider's props to style marks dynamically.
Directly manipulating DOM elements with document.querySelectorAll in a React component is discouraged, as it can lead to unexpected side effects and makes the component harder to maintain. Instead, consider using the Slider's props to style the marks based on the conviction value.
Here's how you can refactor the code:
-
Remove the
useEffectthat manipulates the DOM. -
Modify the
marksdefinition to include dynamic styling:
const marks = useMemo(() =>
- convictionOptions?.map(({ value }) => ({ label: `${value} X`, value: value as number }))
+ convictionOptions?.map(({ value }) => ({
+ value: value as number,
+ label: (
+ <span style={{ color: value > (conviction || DEFAULT_CONVICTION) ? theme.palette.text.disabled : theme.palette.text.primary }}>
+ {`${value} X`}
+ </span>
+ )
+ }))
, [convictionOptions, conviction, theme]);This way, the mark labels are styled dynamically without manipulating the DOM directly.
Committable suggestion skipped: line range outside the PR's diff.
| const match = newText?.match(/\(([^)]+)\)/); | ||
| const result = match ? match[1] : '0 days'; | ||
|
|
||
| return 'Tokens will be locked for ' + result; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Internationalize the entire 'info' string for localization support.
The string 'Tokens will be locked for ' is hard-coded in English. To support localization, the entire string should be wrapped with the translation function.
Apply this diff:
-return 'Tokens will be locked for ' + result;
+return t('Tokens will be locked for {{result}}', { result });Also, update the dependencies in the useMemo hook:
}, [conviction, convictionOptions]);
+}, [conviction, convictionOptions, t]);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (10)
packages/extension-polkagate/src/fullscreen/partials/ModalTitleWithDrag.tsx (4)
4-4: Remove unnecessary ESLint disable comment.The ESLint rule disable for
jsx-max-props-per-linecan be removed by formatting the props across multiple lines where needed.
35-48: Simplify icon rendering logic.The nested ternary operators and empty fragment make the code harder to read. Consider using early returns or a more straightforward conditional structure.
- {icon && - <> - {isIconVaadin - ? <VaadinIcon icon={icon} style={{ color: `${theme.palette.text.primary}`, height: '22px', marginRight: '10px' }} /> - : isIconFontAwesome - ? <FontAwesomeIcon - color={`${theme.palette.text.primary}`} - fontSize='22px' - icon={icon as IconProp} - style={{ marginRight: '10px' }} - /> - : <></> - } - </>} + {isIconVaadin && ( + <VaadinIcon + icon={icon as string} + style={{ color: theme.palette.text.primary, height: '22px', marginRight: '10px' }} + /> + )} + {isIconFontAwesome && ( + <FontAwesomeIcon + color={theme.palette.text.primary} + fontSize='22px' + icon={icon as IconProp} + style={{ marginRight: '10px' }} + /> + )}
33-68: Use MUI's spacing system consistently.The component mixes pixel values with MUI's spacing system. Consider using MUI's spacing consistently for better theme integration and responsiveness.
- <Grid alignItems='center' container justifyContent='space-between' pt='5px'> + <Grid alignItems='center' container justifyContent='space-between' pt={0.625}> <Grid alignItems='center' container item justifyContent='flex-start' width='fit-content'> {/* ... icon rendering ... */} - <Typography display='contents' fontSize='22px' fontWeight={700} pl='10px'> + <Typography display='contents' fontSize={22} fontWeight={700} pl={1.25}> {title} </Typography> </Grid> {/* ... buttons ... */} - <Divider sx={{ mt: '5px', width: '100%' }} /> + <Divider sx={{ mt: 0.625, width: '100%' }} /> </Grid>
25-70: Consider enhancing accessibility.The modal title component could benefit from improved accessibility features:
- Add appropriate ARIA attributes for the draggable region
- Ensure keyboard navigation support for the drag functionality
- Add aria-label to the close button
Example improvements:
<IconButton + aria-label="drag modal" + role="button" + aria-grabbed="false" onMouseDown={onMouseDown} onMouseMove={onMouseMove} onMouseUp={onMouseUp} > {/* ... */} </IconButton> <IconButton + aria-label="close modal" onClick={onClose} > {/* ... */} </IconButton>packages/extension-polkagate/src/fullscreen/governance/components/DraggableModalWithTitle.tsx (2)
13-22: Consider enhancing the Props interface for better type safety.The Props interface could be improved to be more specific and maintainable.
Consider this enhancement:
interface Props { - width?: number; - maxHeight?: number; - minHeight?: number; + width?: number | 500; + maxHeight?: number | 740; + minHeight?: number | 615; children: React.ReactElement; open: boolean; - onClose: () => void + onClose: () => void; title: string; - icon?: string | IconDefinition; + icon?: string | IconDefinition | undefined; }
66-85: Optimize style object definition.The style object is recreated on every render. Consider moving static styles outside the component and only compute dynamic values inside.
Consider this optimization:
+const baseStyle = { + '&:focus': { + outline: 'none' + }, + borderRadius: '10px', + boxShadow: 24, + maxHeight: `${maxHeight}px`, + minHeight: `${minHeight}px`, + pb: 3, + position: 'absolute' as const, + pt: 2, + px: 4, +}; const style = { - '&:focus': { - outline: 'none' - }, + ...baseStyle, bgcolor: 'background.default', border: isDarkMode ? '0.5px solid' : 'none', borderColor: 'secondary.light', - borderRadius: '10px', - boxShadow: 24, cursor: isDragging ? 'grabbing' : 'grab', left: modalPosition.x, - maxHeight: `${maxHeight}px`, - minHeight: `${minHeight}px`, - pb: 3, - position: 'absolute', - pt: 2, - px: 4, top: modalPosition.y, width: `${width}px` };packages/extension-polkagate/src/components/Convictions.tsx (1)
39-44: Improve null safety in info calculation.The current implementation might be vulnerable to undefined values. Consider adding null checks and providing a default value.
const info = useMemo((): string => { - const newText = convictionOptions?.find(({ value }) => value === (conviction || DEFAULT_CONVICTION))?.text; - const match = newText?.match(/\(([^)]+)\)/); + const newText = convictionOptions?.find(({ value }) => value === (conviction || DEFAULT_CONVICTION))?.text ?? ''; + const match = newText.match(/\(([^)]+)\)/); return match ? match[1] : '0 days'; }, [conviction, convictionOptions]);packages/extension-polkagate/src/fullscreen/governance/post/castVote/index.tsx (3)
189-189: Consider adding type safety for the icon propThe migration to
DraggableModalWithTitleimproves component reusability. However, consider adding type safety for the icon prop to ensure only valid FontAwesome icons are passed.- icon={faVoteYea} + icon={faVoteYea as IconDefinition}Also applies to: 275-275
Line range hint
234-244: Remove @ts-ignore and improve type safetyThe use of @ts-ignore and type casting suggests potential type safety issues. Consider properly typing the voteInformation prop to avoid runtime errors.
Consider creating a proper type union:
type ReviewVoteInfo = VoteInformation | (typeof votedInfo); // Then update the Review component prop types: interface ReviewProps { voteInformation: ReviewVoteInfo; // ... other props }
Line range hint
189-275: Consider breaking down the component for better maintainabilityThe component has grown complex with multiple responsibilities and nested conditions. Consider:
- Extracting step-specific components into separate files
- Using a step configuration object to map steps to their respective components
- Implementing a proper state machine for step transitions
Example refactor approach:
const STEP_COMPONENTS = { [STEPS.ABOUT]: About, [STEPS.CHECK_SCREEN]: WaitScreen, // ... other steps } as const; function StepRenderer({ step, props }) { const Component = STEP_COMPONENTS[step]; return Component ? <Component {...props} /> : null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/extension-polkagate/src/components/Convictions.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/governance/components/DraggableModalWithTitle.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/governance/post/castVote/index.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/partials/ModalTitleWithDrag.tsx(1 hunks)
🔇 Additional comments (5)
packages/extension-polkagate/src/components/Convictions.tsx (4)
Line range hint 6-19: LGTM! Well-structured imports and type definitions.
The imports are appropriate for the slider implementation, and the Props interface properly types all required properties. Exporting the DEFAULT_CONVICTION constant is a good practice for maintaining consistency across the application.
28-29: Skip: Array handling issue already reported.
A past review comment already addresses the need to properly handle array values in onChangeConviction.
85-96: LGTM! Well-structured styling implementation.
The styling follows MUI best practices by:
- Using theme colors for consistent styling
- Properly targeting specific Slider components
- Maintaining a clean and maintainable structure
100-109: LGTM! Clean Grid layout implementation.
The Grid layout is well-structured and properly handles the display of locking period information.
packages/extension-polkagate/src/fullscreen/governance/post/castVote/index.tsx (1)
21-21: LGTM: Clean import addition
The new import for DraggableModalWithTitle is properly organized with other component imports.
| interface Props { | ||
| onClose: () => void; | ||
| title: string; | ||
| icon?: string | IconDefinition; | ||
| onMouseDown: (e: { clientX: number; clientY: number; }) => void; | ||
| onMouseMove?: (e: { clientX: number; clientY: number; }) => void; | ||
| onMouseUp?: () => void | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making all mouse event handlers required for consistent drag behavior.
The onMouseMove and onMouseUp handlers are optional while onMouseDown is required. Since these handlers work together to implement dragging functionality, they should all be required to ensure proper drag behavior implementation.
interface Props {
onClose: () => void;
title: string;
icon?: string | IconDefinition;
onMouseDown: (e: { clientX: number; clientY: number; }) => void;
- onMouseMove?: (e: { clientX: number; clientY: number; }) => void;
- onMouseUp?: () => void
+ onMouseMove: (e: { clientX: number; clientY: number; }) => void;
+ onMouseUp: () => void;
}📝 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.
| interface Props { | |
| onClose: () => void; | |
| title: string; | |
| icon?: string | IconDefinition; | |
| onMouseDown: (e: { clientX: number; clientY: number; }) => void; | |
| onMouseMove?: (e: { clientX: number; clientY: number; }) => void; | |
| onMouseUp?: () => void | |
| } | |
| interface Props { | |
| onClose: () => void; | |
| title: string; | |
| icon?: string | IconDefinition; | |
| onMouseDown: (e: { clientX: number; clientY: number; }) => void; | |
| onMouseMove: (e: { clientX: number; clientY: number; }) => void; | |
| onMouseUp: () => void; | |
| } |
| const isIconVaadin = typeof icon === 'string' && icon?.startsWith('vaadin'); | ||
| const isIconFontAwesome = !!icon; | ||
|
|
There was a problem hiding this comment.
Fix incorrect icon type check logic.
The isIconFontAwesome check is incorrect as it only verifies if icon exists, not if it's actually a FontAwesome icon. This could lead to runtime errors if an invalid icon type is provided.
const isIconVaadin = typeof icon === 'string' && icon?.startsWith('vaadin');
-const isIconFontAwesome = !!icon;
+const isIconFontAwesome = typeof icon === 'object' && 'iconName' in icon;📝 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.
| const isIconVaadin = typeof icon === 'string' && icon?.startsWith('vaadin'); | |
| const isIconFontAwesome = !!icon; | |
| const isIconVaadin = typeof icon === 'string' && icon?.startsWith('vaadin'); | |
| const isIconFontAwesome = typeof icon === 'object' && 'iconName' in icon; | |
| const initialX = (window.innerWidth - width) / 2; | ||
| const initialY = (window.innerHeight - maxHeight) / 2; |
There was a problem hiding this comment.
Add window resize handling and SSR safety checks.
The initial position calculation using window.innerWidth/Height has two potential issues:
- It doesn't update when the window is resized
- It may cause issues during SSR as
windowis not available
Consider this enhancement:
+const [windowSize, setWindowSize] = useState({
+ width: typeof window !== 'undefined' ? window.innerWidth : 0,
+ height: typeof window !== 'undefined' ? window.innerHeight : 0
+});
+
+useEffect(() => {
+ if (typeof window === 'undefined') return;
+
+ const handleResize = () => {
+ setWindowSize({
+ width: window.innerWidth,
+ height: window.innerHeight
+ });
+ };
+
+ window.addEventListener('resize', handleResize);
+ return () => window.removeEventListener('resize', handleResize);
+}, []);
-const initialX = (window.innerWidth - width) / 2;
-const initialY = (window.innerHeight - maxHeight) / 2;
+const initialX = (windowSize.width - width) / 2;
+const initialY = (windowSize.height - maxHeight) / 2;📝 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.
| const initialX = (window.innerWidth - width) / 2; | |
| const initialY = (window.innerHeight - maxHeight) / 2; | |
| const [windowSize, setWindowSize] = useState({ | |
| width: typeof window !== 'undefined' ? window.innerWidth : 0, | |
| height: typeof window !== 'undefined' ? window.innerHeight : 0 | |
| }); | |
| useEffect(() => { | |
| if (typeof window === 'undefined') return; | |
| const handleResize = () => { | |
| setWindowSize({ | |
| width: window.innerWidth, | |
| height: window.innerHeight | |
| }); | |
| }; | |
| window.addEventListener('resize', handleResize); | |
| return () => window.removeEventListener('resize', handleResize); | |
| }, []); | |
| const initialX = (windowSize.width - width) / 2; | |
| const initialY = (windowSize.height - maxHeight) / 2; |
| return ( | ||
| <Modal onClose={_onClose} open={open}> | ||
| <Box | ||
| onMouseLeave={handleMouseUp} | ||
| onMouseMove={handleMouseMove} | ||
| onMouseUp={handleMouseUp} | ||
| sx={{ ...style }} | ||
|
|
||
| > | ||
| <ModalTitleWithDrag | ||
| icon={icon} | ||
| onClose={onClose} | ||
| onMouseDown={handleMouseDown} | ||
| title= {title} | ||
| /> | ||
| {children} | ||
| </Box> | ||
| </Modal> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance modal accessibility.
The modal could benefit from improved accessibility features.
Consider these accessibility improvements:
<Modal
+ aria-describedby="modal-description"
+ aria-labelledby="modal-title"
onClose={_onClose}
open={open}
>
<Box
+ role="dialog"
+ tabIndex={-1}
onMouseLeave={handleMouseUp}
onMouseMove={handleMouseMove}
onMouseUp={handleMouseUp}
sx={{ ...style }}
>
<ModalTitleWithDrag
+ id="modal-title"
icon={icon}
onClose={onClose}
onMouseDown={handleMouseDown}
title={title}
/>
- {children}
+ <div id="modal-description">
+ {children}
+ </div>
</Box>
</Modal>📝 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.
| return ( | |
| <Modal onClose={_onClose} open={open}> | |
| <Box | |
| onMouseLeave={handleMouseUp} | |
| onMouseMove={handleMouseMove} | |
| onMouseUp={handleMouseUp} | |
| sx={{ ...style }} | |
| > | |
| <ModalTitleWithDrag | |
| icon={icon} | |
| onClose={onClose} | |
| onMouseDown={handleMouseDown} | |
| title= {title} | |
| /> | |
| {children} | |
| </Box> | |
| </Modal> | |
| ); | |
| return ( | |
| <Modal | |
| aria-describedby="modal-description" | |
| aria-labelledby="modal-title" | |
| onClose={_onClose} | |
| open={open} | |
| > | |
| <Box | |
| role="dialog" | |
| tabIndex={-1} | |
| onMouseLeave={handleMouseUp} | |
| onMouseMove={handleMouseMove} | |
| onMouseUp={handleMouseUp} | |
| sx={{ ...style }} | |
| > | |
| <ModalTitleWithDrag | |
| id="modal-title" | |
| icon={icon} | |
| onClose={onClose} | |
| onMouseDown={handleMouseDown} | |
| title={title} | |
| /> | |
| <div id="modal-description"> | |
| {children} | |
| </div> | |
| </Box> | |
| </Modal> | |
| ); |
| const handleMouseMove = useCallback((e: { clientX: number; clientY: number; }) => { | ||
| if (isDragging) { | ||
| const dx = e.clientX - dragStartPosition.x; | ||
| const dy = e.clientY - dragStartPosition.y; | ||
|
|
||
| setModalPosition((prevPosition) => ({ | ||
| x: prevPosition.x + dx, | ||
| y: prevPosition.y + dy | ||
| })); | ||
| setDragStartPosition({ x: e.clientX, y: e.clientY }); | ||
| } | ||
| }, [dragStartPosition, isDragging]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add bounds checking and cleanup for mouse events.
The drag handling could be improved by:
- Preventing the modal from being dragged outside viewport bounds
- Cleaning up mouse events on component unmount
Consider these improvements:
const handleMouseMove = useCallback((e: { clientX: number; clientY: number; }) => {
if (isDragging) {
const dx = e.clientX - dragStartPosition.x;
const dy = e.clientY - dragStartPosition.y;
+ const newX = Math.max(0, Math.min(window.innerWidth - width, prevPosition.x + dx));
+ const newY = Math.max(0, Math.min(window.innerHeight - maxHeight, prevPosition.y + dy));
+
setModalPosition((prevPosition) => ({
- x: prevPosition.x + dx,
- y: prevPosition.y + dy
+ x: newX,
+ y: newY
}));
setDragStartPosition({ x: e.clientX, y: e.clientY });
}
}, [dragStartPosition, isDragging]);
+useEffect(() => {
+ document.addEventListener('mousemove', handleMouseMove);
+ document.addEventListener('mouseup', handleMouseUp);
+
+ return () => {
+ document.removeEventListener('mousemove', handleMouseMove);
+ document.removeEventListener('mouseup', handleMouseUp);
+ };
+}, [handleMouseMove, handleMouseUp]);Committable suggestion skipped: line range outside the PR's diff.
fix: issue
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/extension-polkagate/src/components/Convictions.tsx (2)
39-44: Consider improving the info text extraction logic.The current regex-based text extraction is fragile. Consider having the
convictionOptionshook return structured data instead.- const info = useMemo((): string => { - const newText = convictionOptions?.find(({ value }) => value === (conviction || DEFAULT_CONVICTION))?.text; - const match = newText?.match(/\(([^)]+)\)/); - - return match ? match[1] : '0 days'; + const info = useMemo((): string => { + const option = convictionOptions?.find(({ value }) => value === (conviction || DEFAULT_CONVICTION)); + return option?.lockPeriod || t('0 days'); }, [conviction, convictionOptions]);
100-109: Improve translation implementation for the lock period display.The current implementation concatenates translated and non-translated parts. Consider using translation variables for better maintainability.
- <Typography sx={{ fontSize: '14px' }}> - {t('Tokens will be locked for')} - </Typography> - </Grid> - <Grid item sx={{ fontSize: '16px', fontWeight: 400 }}> - {info} + <Typography sx={{ fontSize: '14px' }}> + {t('Tokens will be locked for {{period}}', { period: info })} + </Typography>packages/extension-polkagate/src/fullscreen/governance/delegate/index.tsx (1)
Line range hint
144-186: Consider extracting title logic to a separate hookThe title computation logic in the useMemo hook is complex and handles multiple states. Consider extracting it to a custom hook (e.g.,
useDelegateTitle) for better maintainability and reusability.Here's a suggested refactor:
const useDelegateTitle = (step: number, status: DelegationStatus, txInfo?: TxInfo) => { const { t } = useTranslation(); return useMemo(() => { switch (step) { case STEPS.ABOUT: return t('Delegate Vote'); case STEPS.CHECK_SCREEN: return t('Delegation status'); case STEPS.INDEX: case STEPS.CHOOSE_DELEGATOR: return t('Delegate Vote ({{ step }}/3)', { replace: { step: step === STEPS.INDEX ? 1 : 2 } }); // ... rest of the cases default: return ''; } }, [status, step, t, txInfo?.success]); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/extension-polkagate/src/components/Convictions.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/governance/components/DraggableModalWithTitle.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/governance/delegate/index.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/partials/ModalTitleWithDrag.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/extension-polkagate/src/fullscreen/governance/components/DraggableModalWithTitle.tsx
- packages/extension-polkagate/src/fullscreen/partials/ModalTitleWithDrag.tsx
🔇 Additional comments (5)
packages/extension-polkagate/src/components/Convictions.tsx (3)
6-7: LGTM: Imports and constant definitions are well-structured.
The imports are appropriate for the slider implementation, and the DEFAULT_CONVICTION constant is properly exported.
Also applies to: 19-20
32-37: LGTM: Clean implementation of min/max calculation.
The useMemo hook efficiently calculates min/max values with proper type assertions and fallback values.
85-96: LGTM: Well-structured slider styling.
The custom styling is cleanly implemented using MUI's sx prop with proper color theming.
packages/extension-polkagate/src/fullscreen/governance/delegate/index.tsx (2)
23-23: LGTM: Clean import addition
The new import for DraggableModalWithTitle is properly organized with other component imports.
288-293: Verify icon consistency across steps
The modal implementation looks good with proper prop handling. However, the icon selection logic uses a mix of Font Awesome and Vaadin icons. Consider standardizing the icon library usage for better maintainability.
Let's check for icon consistency across the codebase:
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information about icon usage patterns in the codebase:
Icon usage is consistent within component contexts
The codebase shows a clear pattern of using specific icon libraries for specific purposes:
- Font Awesome icons are used for action/functional buttons and status indicators
- Vaadin icons are used primarily for navigation, menus and modal headers
The mixed usage in the delegate modal is actually consistent with this pattern - faUserAstronaut for the proxy step (functional) and vaadin:money-withdraw for the transaction step (modal header).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for mixed icon usage patterns
# Look for Font Awesome and Vaadin icon imports/usage
rg -n "fa[A-Z]|'vaadin:" packages/extension-polkagate/src/
Length of output: 77456
# [0.22.0](v0.21.6...v0.22.0) (2024-11-06) ### Features * use Slider for Conviction settings ([#1629](#1629)) ([790d7a1](790d7a1))
Summary by CodeRabbit
Release Notes
New Features
Convictionscomponent with a new slider for selecting conviction levels, improving user interaction.DraggableModalWithTitlecomponent for a more user-friendly modal experience.Bug Fixes
Documentation
Style
Delegatecomponent for improved readability.