Fix (#950) Steps for purchasing a domain #995
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@Viktohblake is attempting to deploy a commit to the LFG Labs Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces comprehensive changes across multiple components and styles, focusing on redesigning the registration and purchase flow. The modifications include updating icon components, restructuring the registration steps, enhancing responsive design, and introducing new CSS variables and styles. The changes aim to improve the user interface and experience for domain registration, with a particular emphasis on creating a more structured and visually coherent step-by-step process. Changes
Assessment against linked issues
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
styles/components/upsellCard.module.css (1)
Line range hint
101-105: Review radio group spacing for better usability.Setting
gap: 0pxin the radio group might make options too cramped and harder to tap on mobile devices. Consider:
- Maintaining some vertical spacing
- Adding padding for better touch targets
.radioGroup { flex-direction: column; - gap: 0px; + gap: 12px; align-items: center; justify-content: center;
🧹 Nitpick comments (6)
styles/components/upsellCard.module.css (3)
76-77: Review container margin and width settings.The combination of
max-width: 90%andmargin: 1remcould cause horizontal scrolling on smaller screens. Consider using either:
width: calc(100% - 2rem)withmargin: 1rem, orwidth: 90%withmargin: 1rem auto.container { - max-width: 90%; - width: 100%; - margin: 1rem; + width: calc(100% - 2rem); + margin: 1rem;
Line range hint
89-93: Consider image aspect ratio and loading optimization.The fixed height on the image might cause aspect ratio issues or image distortion. Consider:
- Using aspect-ratio property
- Adding loading="lazy" to the image element
- Adding srcset for responsive images
.image { order: 1; width: 100%; - height: 220px; + aspect-ratio: 16/9; position: relative;
Line range hint
1-17: Clean up duplicate properties and simplify z-index usage.The base
.containerclass has:
- Duplicate
justify-content: space-between- Potentially unnecessary z-index stacking context
.container { display: flex; flex-direction: row; align-items: center; - justify-content: space-between; width: 1084px; max-width: 100%; background-color: white; justify-content: space-between; margin-bottom: 100px; border-radius: 1rem; box-shadow: 0px 2px 30px 0px rgba(0, 0, 0, 0.06); margin-bottom: 3rem; overflow: hidden; position: relative; - z-index: 0; }components/domains/registerV3.tsx (1)
60-105: Consider using semantic HTML elements.The layout structure could benefit from more semantic HTML elements:
asideis correctly used for the step navigation- Consider using
mainfor the main content area- <div className="flex flex-col md:flex-row gap-10"> + <div className="flex flex-col md:flex-row gap-10" role="main"> <aside className={styles.purchaseStepNav}> {/* ... */} </aside> - <div className="flex-1"> + <main className="flex-1"> {/* ... */} - </div> + </main> </div>components/domains/steps/registerSteps.tsx (1)
17-27: Improve type safety of helper functions.The helper functions could benefit from TypeScript enums and stricter types.
+enum StepState { + PASSED = 'passedStep', + ACTIVE = 'activeStep', + DISABLED = 'disabledStep', +} -const getStep = (currentStep: number, stepIndex: number) => { +const getStep = (currentStep: number, stepIndex: number): StepState => { - if (currentStep > stepIndex) return styles.passedStep; - if (currentStep === stepIndex) return styles.activeStep; - return styles.disabledStep; + if (currentStep > stepIndex) return StepState.PASSED; + if (currentStep === stepIndex) return StepState.ACTIVE; + return StepState.DISABLED; };styles/components/registerV3.module.css (1)
243-261: Consider using CSS custom properties for breakpoints.The media query breakpoints could be more maintainable using CSS custom properties.
+:root { + --breakpoint-md: 768px; + --breakpoint-lg: 1024px; +} -@media (min-width: 768px) { +@media (min-width: var(--breakpoint-md)) { /* ... */ } -@media (min-width: 1024px) { +@media (min-width: var(--breakpoint-lg)) { /* ... */ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
public/visuals/purchaseStepVisual.svgis excluded by!**/*.svgpublic/visuals/purchaseStepVisualMobile.svgis excluded by!**/*.svg
📒 Files selected for processing (8)
components/UI/iconsComponents/icons/cartIcon.tsx(1 hunks)components/UI/iconsComponents/icons/contactCardIcon.tsx(1 hunks)components/UI/iconsComponents/icons/pfpIcon.tsx(1 hunks)components/domains/registerV3.tsx(3 hunks)components/domains/steps/registerSteps.tsx(3 hunks)styles/components/registerV3.module.css(3 hunks)styles/components/upsellCard.module.css(1 hunks)styles/theme.ts(1 hunks)
🔇 Additional comments (8)
components/UI/iconsComponents/icons/contactCardIcon.tsx (2)
6-11: LGTM! SVG container attributes are well-structured.The changes to make the icon square (equal width and height) and the new viewBox size of 16x16 align well with modern icon design practices. This promotes consistency across the icon system.
13-14: LGTM! Path implementation is clean and supports dynamic coloring.The path data is well-formed and properly scaled for the 16x16 viewBox. The dynamic color prop usage allows for flexible theming.
components/UI/iconsComponents/icons/cartIcon.tsx (2)
15-17: LGTM! Clean SVG path implementation.The new path data provides a well-structured cart icon with appropriate detail level, including the cart body and wheels.
9-13: Verify icon scaling consistency.The
viewBoxhas been changed from24x24to16x16, which affects the icon's scaling. This change should be intentional and consistent with the design system.✅ Verification successful
Icon scaling change is consistent with design system ✓
The change to
viewBox="0 0 16 16"aligns with the standard icon size used across most UI icons in the codebase. This appears to be an intentional standardization.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check viewBox values across icon components rg 'viewBox="0 0 \d+ \d+"' 'components/UI/iconsComponents'Length of output: 5026
components/UI/iconsComponents/icons/pfpIcon.tsx (3)
15-31: LGTM! Clean and consistent path styling.The SVG paths are well-structured and use consistent styling with the
colorprop. The removal of the background rect and consolidation to a single color scheme simplifies the implementation while maintaining the icon's visual clarity.
3-6: Verify the removal of secondColor prop across the codebase.The removal of the
secondColorprop suggests a simplification of the icon's color scheme. Let's ensure this change doesn't break any existing usages.✅ Verification successful
The removal of secondColor prop is safe ✅
The only usage of PfpIcon in the codebase (in registerSteps.tsx) already uses the correct props structure (width and color only), making this change backward compatible.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of secondColor prop with PfpIcon rg -l "PfpIcon.*secondColor" # Search for any imports of PfpIcon to analyze usage rg -l "import.*PfpIcon"Length of output: 99
Script:
#!/bin/bash # Check how PfpIcon is used in registerSteps.tsx rg -A 5 "PfpIcon" components/domains/steps/registerSteps.tsx # Find and examine IconProps interface ast-grep --pattern 'interface IconProps { $$$ }'Length of output: 598
9-14: Verify the visual impact of the viewBox change.The viewBox has been changed from 24x24 to 16x16, which affects the icon's scaling behavior. Please ensure this change:
- Maintains visual consistency with other icons
- Renders correctly at all required sizes
- Aligns with the design system specifications
✅ Verification successful
ViewBox change is consistent with design system
The change from 24x24 to 16x16 viewBox for pfpIcon aligns with the most common viewBox dimensions used across similar UI icons in the codebase. Most functional UI icons use either 16x16 or 16x17 viewBox, while only special icons like social media and wallet icons use larger dimensions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for viewBox patterns in other icon components rg 'viewBox="0 0 \d+ \d+"' --type tsx --glob '*Icon.tsx'Length of output: 86
Script:
#!/bin/bash # Search for viewBox patterns across all files rg -i 'viewBox="0 0 \d+ \d+"' --glob '*.{tsx,svg}' # List all icon-related files to ensure we're searching in the right places fd -e tsx -e svg iconLength of output: 67298
Script:
#!/bin/bash # Search for SVG viewBox and dimensions in icon components directory cd components/UI/iconsComponents/icons 2>/dev/null && rg -i 'viewBox="0 0 \d+ \d+"' . # List all icon files to verify we're in the right directory cd components/UI/iconsComponents/icons 2>/dev/null && ls -laLength of output: 2503
styles/theme.ts (1)
17-17: LGTM! Good addition to the theme palette.The new grey shade (#CDCCCC) provides appropriate contrast for disabled states in the step navigation.
There was a problem hiding this comment.
As on figma, the steps card should fill the screen vertically (with the same margins as on figma). Make sur to have the same dimensions.
Also, please read all the comments made by the ai, it's mostly right.
In addition, don't forget to fix the conflicts often (by merging the testnet branch in yours), because all the cards for purchasing a domain are being redesigned, and your job will be to make them fit with your new steps card.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
pages/identities.tsx (1)
Line range hint
123-142: Reduce code duplication in button implementation.The
AddButtonimplementation is duplicated. Consider extracting it into a reusable component to maintain DRY principles.+ const IdentityAddButton = () => ( + <AddButton + onClick={address ? () => mint() : () => setShowWalletConnectModal(true)} + radius="8px" + > + ADD IDENTITIES + </AddButton> + ); return ( <> <div className={styles.containerGallery}> <div> {loading ? ( <IdentitiesSkeleton /> ) : ownedIdentities.length + externalDomains.length === 0 || !address ? ( <> ... - <div className="w-fit block mx-auto px-4 mt-[48px] "> - <AddButton onClick={ - address - ? () => mint() - : () => setShowWalletConnectModal(true) - } radius="8px"> - ADD IDENTITIES - </AddButton> - </div> + <div className="w-fit block mx-auto px-4 mt-[48px] "> + <IdentityAddButton /> + </div> </> ) : ( <div> <IdentitiesGallery identities={ownedIdentities} externalDomains={externalDomains} address={address} /> - <div className="w-fit block mx-auto px-4 mt-[48px] bg-red-500"> - <AddButton onClick={ - address - ? () => mint() - : () => setShowWalletConnectModal(true) - } radius="8px"> - ADD IDENTITIES - </AddButton> - </div> + <div className="w-fit block mx-auto px-4 mt-[48px]"> + <IdentityAddButton /> + </div> </div> )}Also applies to: 152-170
♻️ Duplicate comments (1)
styles/components/registerV3.module.css (1)
315-317:⚠️ Potential issueFix accessibility: Insufficient color contrast for disabled text.
The disabled text color (#CDCCCC) has a contrast ratio of 1.60:1 against white background, which fails WCAG accessibility standards (minimum 4.5:1 required).
.disabledStep { - color: var(--disabled, #CDCCCC); + color: var(--disabled, #767676); cursor: not-allowed; }
🧹 Nitpick comments (22)
components/UI/textField.tsx (1)
57-69: Consider extracting style constants.While the implementation is solid, consider extracting the hard-coded values to theme constants for better maintainability:
- Box shadow: "0px 2px 30px 0px #0000000F"
- Height: "56px"
- Colors: "#FFFFFF", "#CDCCCC", "#CDCCDD"
Example approach:
// theme/constants.ts export const TEXTFIELD_STYLES = { boxShadow: '0px 2px 30px 0px #0000000F', height: '56px', backgroundColor: '#FFFFFF', borderColor: { default: '#CDCCCC', hover: '#CDCCDD' } } as const;styles/components/textField.module.css (1)
28-30: LGTM! Consider increasing error state visibility.The font adjustments improve readability. However, since the font weight is now lighter (400), consider making the error state more prominent.
.errorLegend { color: red; + font-weight: 500; }tailwind.config.js (1)
11-11: Add space after colon for consistent formatting.The new color entry lacks consistent spacing compared to other color definitions.
- "background-nav":"#FCFFFE" + "background-nav": "#FCFFFE"styles/components/button.module.css (1)
Line range hint
116-120: LGTM! Consider documenting the breakpoint choice.The responsive adjustments look good. The line-height to font-size ratio (≈1.71) maintains good readability. Consider adding a comment explaining why 1024px was chosen as the breakpoint for future maintainers.
+/* Tablet and smaller devices */ @media (max-width: 1024px) {components/domains/steps/upsellCard.tsx (1)
46-73: LGTM! Consider adding ARIA label for better accessibility.The UI restructuring improves visual hierarchy and organization. The radio group implementation is clean and follows Material-UI best practices.
Consider adding an
aria-labelto the radio group for better screen reader support:<RadioGroup - aria-labelledby="demo-controlled-radio-buttons-group" + aria-label="Upsell options" name="controlled-radio-buttons-group" value={enabled} onChange={handleUpsellChoice} className={styles.radioGroupContainer} >pages/newsletter.tsx (1)
63-63: Remove commented code.Instead of commenting out the helper text, either remove it entirely or keep it active. Commented code can lead to confusion and maintenance issues.
- // helperText="Your email stays private with us, always."components/domains/steps/checkoutCard.tsx (3)
253-263: Consider making the condition more descriptiveThe condition
formState.durationInYears === 1could be more descriptive to explain why the upsell is only shown for 1-year registrations.- {formState.durationInYears === 1 ? ( + {formState.durationInYears === 1 && discount ? ( // Show upsell only for 1-year registrations when discount is available
301-330: Simplify the disabled state logicThe disabled state logic could be simplified by extracting it into a separate function for better readability and maintainability.
+ const isButtonDisabled = () => { + return ( + domainsMinting === formState.selectedDomains || + !account || + !formState.durationInYears || + formState.durationInYears < 1 || + invalidBalance || + !termsBox + ); + }; + <Button onClick={() => execute().then(() => { setDomainsMinting(formState.selectedDomains); }) } - disabled={ - domainsMinting === formState.selectedDomains || - !account || - !formState.durationInYears || - formState.durationInYears < 1 || - invalidBalance || - !termsBox - } + disabled={isButtonDisabled()} >
334-340: Add accessibility attributes to the close buttonThe close button should have proper accessibility attributes for better screen reader support.
<button onClick={() => router.push("/")} + aria-label="Close checkout" + className="p-1 hover:opacity-80 transition-opacity" > <CloseIcon /> </button>components/UI/navbar.tsx (1)
166-169: Use Next.js Image component for better performanceConsider using Next.js's Image component for automatic image optimization, lazy loading, and better performance.
-<img +<Image className={styles.starknetIdLogo} src={isMobile ? "/visuals/MbLogo.svg" : "/visuals/Logo.svg"} alt="Starknet.id Logo" width={isMobile ? 48 : 170} height={isMobile ? 48 : 90} + priority />styles/verify.module.css (1)
13-13: Keep the fallback color for robustnessRemoving the fallback color could cause styling issues if the CSS variable is undefined.
- color: var(--content-600); + color: var(--content-600, #8c8989);.env.example (1)
14-14: Consider environment-specific configuration.The
NEXT_PUBLIC_IS_TESTNET=falsesetting should be managed through deployment configuration rather than hardcoded in the example file.-NEXT_PUBLIC_IS_TESTNET=false +NEXT_PUBLIC_IS_TESTNET=styles/components/upsellCard.module.css (1)
59-63: Consider accessibility improvements for the radio group.The radio group container needs focus state styles for better keyboard navigation.
.radioGroupContainer { width: 100%; border: 1px solid #45454533; border-radius: 8px; padding: 8px 20px; + transition: border-color 0.2s ease; } +.radioGroupContainer:focus-within { + border-color: var(--primary); + outline: none; +}styles/components/addIdentitiesButton.module.css (3)
8-8: Fix CSS property formatting.Add spaces after colons in CSS properties for better readability.
- height:44px; + height: 44px; - width:200px; + width: 200px;Also applies to: 18-18
47-48: Avoid using !important.The use of
!importantshould be avoided as it breaks CSS specificity and makes styles harder to maintain.Consider restructuring the selectors to achieve the desired specificity without using
!important.
91-113: Enhance focus ring accessibility.Consider using
:focus-visibleinstead of:focusfor the focus ring to improve keyboard navigation accessibility while maintaining a clean visual experience for mouse users.-.iq-button:focus::after { +.iq-button:focus-visible::after { opacity: 1; }styles/components/newsletter.module.css (2)
71-71: Use CSS variable for color consistency.Replace hardcoded color value with a CSS variable to maintain consistency with the design system.
- color: #8C8989; + color: var(--content-600);
162-169: Extract magic numbers into CSS variables.Consider extracting the positioning values into CSS variables for better maintainability and consistency.
+:root { + --tree1-offset-left: -114px; + --tree1-offset-bottom: -105px; + --tree2-offset-right: -140.75px; + --tree2-offset-bottom: -78px; +} .tree1 { - left: -114px; - bottom: -105px; + left: var(--tree1-offset-left); + bottom: var(--tree1-offset-bottom); } .tree2 { - right: -140.75px; - bottom: -78px; + right: var(--tree2-offset-right); + bottom: var(--tree2-offset-bottom); }components/domains/registerV3.tsx (1)
60-105: Consider extracting step content into separate components.The step content rendering logic could be extracted into separate components to improve maintainability and reusability.
Consider creating separate components for each step:
UserInfoStepPfpSelectionStepCheckoutStepThis would make the code more modular and easier to maintain.
components/domains/steps/registerSteps.tsx (1)
18-28: Improve type safety and performance of helper functions.The helper functions could benefit from better type safety and memoization.
+type StepNumber = 1 | 2 | 3; -const getStep = (currentStep: number, stepIndex: number) => { +const getStep = (currentStep: StepNumber, stepIndex: StepNumber): string => { if (currentStep > stepIndex) return styles.passedStep; if (currentStep === stepIndex) return styles.activeStep; return styles.disabledStep; }; -const getStepColor = (currentStep: number, step: number): string => { +const getStepColor = (currentStep: StepNumber, step: StepNumber): string => { if (currentStep > step) return theme.palette.primary.main; if (currentStep === step) return theme.palette.secondary.main; return theme.palette.grey[200]; }; +const memoizedGetStep = React.useMemo(() => getStep, []); +const memoizedGetStepColor = React.useMemo(() => getStepColor, []);pages/identities.tsx (1)
Line range hint
124-133: Remove commented code.Remove the commented-out
ClickableActioncomponent code instead of leaving it in place. This helps maintain code cleanliness.styles/components/profilePic.module.css (1)
249-293: Consider adding prefers-reduced-motion media query.For users who prefer reduced motion, consider adding a media query to disable transitions.
+ @media (prefers-reduced-motion: reduce) { + .nftImg { + transition: none; + } + }
🛑 Comments failed to post (3)
components/UI/AddButtonIdentities.tsx (1)
1-31: 🛠️ Refactor suggestion
Consider refactoring to avoid code duplication.
This component is nearly identical to the base
Buttoncomponent, with the only differences being the styles import path and CSS class name. Consider refactoring to reuse the baseButtoncomponent instead:
- Move the styles to a shared location or
- Add a
classNameprop to the baseButtoncomponent to allow style overrides.Example refactor approach:
- // Remove this entire file - import React, { FunctionComponent, ReactNode } from "react"; - import styles from "../../styles/components/addIdentitiesButton.module.css"; - - type ButtonProps = { - onClick: () => void; - children: string | ReactNode; - disabled?: boolean; - variation?: string; - radius?: string; - }; - - const AddButton: FunctionComponent<ButtonProps> = ({ - children, - onClick, - disabled = false, - variation = "primary", - radius, - }) => { - return ( - <button - disabled={disabled} - onClick={onClick} - className={` ${styles["iq-button"]} ${styles[variation]}`} - style={radius ? { borderRadius: radius } : undefined} - > - {children} - </button> - ); - }; - - export default AddButton; + // In button.tsx, modify the base Button component: + type ButtonProps = { + onClick: () => void; + children: string | ReactNode; + disabled?: boolean; + variation?: string; + radius?: string; + className?: string; // Add this prop + }; + + const Button: FunctionComponent<ButtonProps> = ({ + children, + onClick, + disabled = false, + variation = "primary", + radius, + className, // Add this prop + }) => { + return ( + <button + disabled={disabled} + onClick={onClick} + className={`${styles["nq-button"]} ${styles[variation]} ${className}`} + style={radius ? { borderRadius: radius } : undefined} + > + {children} + </button> + ); + };Committable suggestion skipped: line range outside the PR's diff.
styles/components/upsellCard.module.css (1)
72-75: 💡 Codebase verification
The divider is explicitly hidden on mobile without alternative visual separation
The code confirms that the divider is set to
display: noneon screens smaller than 640px, which could impact content readability and visual hierarchy. Consider removing the mobile-specific style to maintain consistent visual separation across all screen sizes.🔗 Analysis chain
Consider retaining divider on mobile.
The divider is hidden on mobile screens (<640px), but visual separation might still be beneficial for clarity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are other visual separators in mobile view rg -l "border|margin|padding" styles/components/ | xargs rg "@media.*max-width: 640px"Length of output: 246
Script:
#!/bin/bash # Show complete media query blocks and surrounding context rg -B 2 -A 5 "@media.*max-width: 640px" styles/components/upsellCard.module.css # Look for .divider class references rg "\.divider" styles/components/upsellCard.module.cssLength of output: 243
styles/globals.css (1)
28-28: 💡 Codebase verification
Color contrast ratios do not meet WCAG 2.1 requirements for normal text
The current contrast ratios:
- Content on background (#8c8989 on #fcfffe): 3.45:1
- Content on white (#8c8989 on #ffffff): 3.47:1
While these meet the requirements for large text (>3:1), they fall short of the minimum 4.5:1 ratio needed for normal text. Consider using a darker shade for the content color (--content-600) to improve readability.
🔗 Analysis chain
Verify color contrast ratios for accessibility.
Please ensure the new colors meet WCAG 2.1 contrast requirements:
--background: #fcfffe--background-white: #ffffff--content-600: #8c8989Also applies to: 31-31, 34-34
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Install and run color contrast checker pip install colormath python << EOF from colormath.color_objects import sRGBColor, LabColor from colormath.color_conversions import convert_color from colormath.color_diff import delta_e_cie2000 def hex_to_rgb(hex_color): hex_color = hex_color.lstrip('#') return tuple(int(hex_color[i:i+2], 16)/255 for i in (0, 2, 4)) def calculate_contrast(color1, color2): rgb1 = sRGBColor(*hex_to_rgb(color1)) rgb2 = sRGBColor(*hex_to_rgb(color2)) lab1 = convert_color(rgb1, LabColor) lab2 = convert_color(rgb2, LabColor) return delta_e_cie2000(lab1, lab2) # Check contrast ratios colors = { 'background': '#fcfffe', 'content': '#8c8989', 'background_white': '#ffffff' } print("Contrast Ratios:") print(f"content on background: {calculate_contrast(colors['content'], colors['background'])}") print(f"content on background_white: {calculate_contrast(colors['content'], colors['background_white'])}") EOFLength of output: 2018
Script:
#!/bin/bash python << EOF def get_luminance(hex_color): # Remove # if present hex_color = hex_color.lstrip('#') # Convert hex to RGB r = int(hex_color[0:2], 16) / 255.0 g = int(hex_color[2:4], 16) / 255.0 b = int(hex_color[4:6], 16) / 255.0 # Convert to sRGB r = r / 12.92 if r <= 0.03928 else ((r + 0.055) / 1.055) ** 2.4 g = g / 12.92 if g <= 0.03928 else ((g + 0.055) / 1.055) ** 2.4 b = b / 12.92 if b <= 0.03928 else ((b + 0.055) / 1.055) ** 2.4 # Calculate luminance return 0.2126 * r + 0.7152 * g + 0.0722 * b def calculate_contrast_ratio(color1, color2): l1 = get_luminance(color1) l2 = get_luminance(color2) # Ensure lighter color is l1 if l2 > l1: l1, l2 = l2, l1 return (l1 + 0.05) / (l2 + 0.05) # Colors to test content = '#8c8989' background = '#fcfffe' background_white = '#ffffff' # Calculate ratios content_on_bg = calculate_contrast_ratio(content, background) content_on_white = calculate_contrast_ratio(content, background_white) print(f"WCAG Contrast Ratios:") print(f"Content on background: {content_on_bg:.2f}:1") print(f"Content on white: {content_on_white:.2f}:1") print(f"\nWCAG 2.1 Requirements:") print("- Normal text: minimum 4.5:1") print("- Large text: minimum 3:1") EOFLength of output: 164
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
styles/components/registerV3.module.css (1)
324-326:⚠️ Potential issueFix color contrast for disabled text.
The disabled text color (#CDCCCC) has insufficient contrast against white background, failing WCAG accessibility standards.
.disabledStep { - color: var(--disabled, #CDCCCC); + color: var(--disabled, #767676); cursor: not-allowed; }
🧹 Nitpick comments (2)
styles/components/registerV3.module.css (2)
414-417: Consider handling horizontal overflow.While the horizontal layout works well for the steps container on mobile, you might want to handle potential overflow to prevent horizontal scrolling of the entire page.
.stepsContainer { min-width: 200px; flex-direction: row; justify-content: center; justify-self: center; + overflow-x: auto; + -webkit-overflow-scrolling: touch; }
264-264: Consider documenting breakpoint choices.For better maintainability, consider adding comments to explain the purpose of each breakpoint (desktop, tablet, mobile, etc.).
+/* Desktop breakpoint */ @media (min-width: 768px) { +/* Large screens */ @media (max-width: 1570px) { +/* Tablet and smaller */ @media (max-width: 1084px) { +/* Mobile */ @media screen and (max-width: 768px) { +/* Small mobile */ @media screen and (max-width: 640px) {Also applies to: 370-370, 404-404, 426-426, 438-438
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
styles/components/registerV3.module.css(6 hunks)
🔇 Additional comments (2)
styles/components/registerV3.module.css (2)
4-4: LGTM! Improved container responsiveness.The change to
width: 100%enhances the layout's fluidity across different screen sizes.
250-281: Well-structured responsive navigation implementation!Good use of mobile-first approach with clear separation between mobile and desktop views. The implementation properly handles different screen sizes using standard breakpoints.
|
hi, check for the changes now, let me know if there's anything else. 
|
|
But overall it looks good, well done |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
components/domains/registerV3.tsx (1)
60-89: Improve component reuse and accessibility.The current implementation duplicates the
RegisterStepscomponent for desktop and mobile views. Consider using CSS media queries to control visibility instead of component duplication.- <aside className={`${styles.purchaseStepNav}`}> + <aside className={`${styles.purchaseStepNav}`} role="navigation"> <RegisterSteps currentStep={currentStep} setStep={goToStep} showPfp={userNfts && userNfts.length > 0} isLoading={isLoadingNfts} /> <img src="/visuals/purchaseStepVisual.svg" alt="Domain purchase steps visualization" /> </aside> - <div className={`${styles.purchaseStepNavMobile}`}> - <RegisterSteps - currentStep={currentStep} - setStep={goToStep} - showPfp={userNfts && userNfts.length > 0} - isLoading={isLoadingNfts} - /> - <div className="flex justify-center"> - <img - src="/visuals/purchaseStepVisualMobile.svg" - alt="Domain purchase steps visualization" - /> - </div> - </div>components/domains/steps/registerSteps.tsx (1)
18-28: Add type safety to helper functions.The helper functions could benefit from more specific TypeScript types.
-const getStep = (currentStep: number, stepIndex: number) => { +const getStep = (currentStep: number, stepIndex: number): string => { if (currentStep > stepIndex) return styles.passedStep; if (currentStep === stepIndex) return styles.activeStep; return styles.disabledStep; }; +type StepColor = typeof theme.palette.primary.main | typeof theme.palette.secondary.main | typeof theme.palette.grey[200]; const getStepColor = (currentStep: number, step: number): string => { if (currentStep > step) return theme.palette.primary.main; if (currentStep === step) return theme.palette.secondary.main; return theme.palette.grey[200]; };styles/components/registerV3.module.css (1)
Line range hint
403-493: Optimize responsive styles to reduce repetition.The mobile styles contain repeated padding rules across different breakpoints. Consider using CSS custom properties to improve maintainability.
+:root { + --step-padding-mobile: 5px; + --step-padding-small: 0px; +} @media screen and (max-width: 768px) { .passedStep, .activeStep, .disabledStep { - padding: 5px; + padding: var(--step-padding-mobile); } } @media screen and (max-width: 400px) { .passedStep, .activeStep, .disabledStep { - padding: 0px; + padding: var(--step-padding-small); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/domains/registerV3.tsx(2 hunks)components/domains/steps/registerSteps.tsx(1 hunks)styles/components/registerV3.module.css(7 hunks)
🧰 Additional context used
📓 Learnings (2)
components/domains/registerV3.tsx (1)
Learnt from: Viktohblake
PR: lfglabs-dev/app.starknet.id#995
File: components/domains/registerV3.tsx:19-21
Timestamp: 2025-01-26T19:18:53.841Z
Learning: In the `RegisterV3` component (`components/domains/registerV3.tsx`), the `setDomain` prop is kept despite being unused because removing it would require changes to dependent files. This prop is marked with a comment indicating it's not used since the domain search bar was removed.
styles/components/registerV3.module.css (1)
Learnt from: Viktohblake
PR: lfglabs-dev/app.starknet.id#995
File: styles/components/registerV3.module.css:0-0
Timestamp: 2025-01-26T19:24:53.180Z
Learning: While maintaining design fidelity is important, accessibility requirements (like WCAG color contrast standards) should take precedence. When implementing designs from Figma, validate color contrast ratios and suggest accessible alternatives that maintain the design intent.
🔇 Additional comments (3)
components/domains/registerV3.tsx (1)
91-106: LGTM! Well-structured form rendering logic.The step-based form rendering is well-organized with clear component separation.
components/domains/steps/registerSteps.tsx (1)
37-56: LGTM! Well-implemented loading state with accessibility.The loading state implementation uses proper skeleton components and includes appropriate ARIA attributes.
styles/components/registerV3.module.css (1)
313-315:⚠️ Potential issueImprove color contrast for disabled steps.
The disabled step color (#CDCCCC) has insufficient contrast against the white background (ratio: 1.60:1), failing WCAG accessibility standards (minimum 4.5:1 required).
.disabledStep { - color: var(--disabled, #CDCCCC); + color: var(--disabled, #767676); cursor: not-allowed; }Likely invalid or redundant comment.
|
Hello @Marchand-Nicolas, I have checked everything, I believe we are good to go. Review and let me know if any other thing. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
components/domains/registerV3.tsx (2)
59-72: Improve accessibility of the navigation structure.While the
role="navigation"is correctly added, the navigation can be enhanced for better accessibility.Consider these improvements:
- <aside className={`${styles.purchaseStepNav}`} role="navigation"> + <aside className={`${styles.purchaseStepNav}`} role="navigation" aria-label="Purchase steps"> <RegisterSteps currentStep={currentStep} setStep={goToStep} showPfp={userNfts && userNfts.length > 0} isLoading={isLoadingNfts} /> <img src="/visuals/purchaseStepVisual.svg" alt="Domain purchase steps visualization" + role="presentation" /> </aside>
74-88: Maintain consistent accessibility attributes in mobile navigation.The mobile navigation should maintain the same accessibility standards as the desktop version.
Apply similar improvements to the mobile navigation:
- <div className={`${styles.purchaseStepNavMobile}`} role="navigation"> + <div className={`${styles.purchaseStepNavMobile}`} role="navigation" aria-label="Purchase steps"> <RegisterSteps currentStep={currentStep} setStep={goToStep} showPfp={userNfts && userNfts.length > 0} isLoading={isLoadingNfts} /> <div className="flex justify-center"> <img src="/visuals/purchaseStepVisualMobile.svg" alt="Domain purchase steps visualization" + role="presentation" /> </div> </div>styles/components/registerV3.module.css (1)
Line range hint
358-520: Optimize media query breakpoints.The media queries could be more maintainable by using CSS variables for breakpoints.
Consider defining breakpoints as CSS variables at the root:
:root { --breakpoint-sm: 640px; --breakpoint-md: 768px; --breakpoint-lg: 1084px; --breakpoint-xl: 1570px; }Then use them in media queries:
-@media (max-width: 1570px) { +@media (max-width: var(--breakpoint-xl)) { /* styles */ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/domains/registerV3.tsx(2 hunks)styles/components/button.module.css(0 hunks)styles/components/registerV3.module.css(8 hunks)
💤 Files with no reviewable changes (1)
- styles/components/button.module.css
🧰 Additional context used
📓 Learnings (2)
components/domains/registerV3.tsx (1)
Learnt from: Viktohblake
PR: lfglabs-dev/app.starknet.id#995
File: components/domains/registerV3.tsx:19-21
Timestamp: 2025-01-26T19:18:53.841Z
Learning: In the `RegisterV3` component (`components/domains/registerV3.tsx`), the `setDomain` prop is kept despite being unused because removing it would require changes to dependent files. This prop is marked with a comment indicating it's not used since the domain search bar was removed.
styles/components/registerV3.module.css (1)
Learnt from: Viktohblake
PR: lfglabs-dev/app.starknet.id#995
File: styles/components/registerV3.module.css:0-0
Timestamp: 2025-01-26T19:24:53.180Z
Learning: While maintaining design fidelity is important, accessibility requirements (like WCAG color contrast standards) should take precedence. When implementing designs from Figma, validate color contrast ratios and suggest accessible alternatives that maintain the design intent.
🔇 Additional comments (4)
components/domains/registerV3.tsx (2)
8-8: LGTM! CSS module import added correctly.The CSS module import follows best practices for modular styling.
90-104: LGTM! Form content rendering is well-structured.The conditional rendering of form steps is clean and follows best practices.
styles/components/registerV3.module.css (2)
244-274: LGTM! Responsive navigation layout implemented correctly.The desktop/mobile toggle using media queries is well-implemented.
311-313:⚠️ Potential issueFix color contrast for disabled steps.
The disabled text color (#CDCCCC) has insufficient contrast against the white background, making it hard to read for users with visual impairments.
Apply this change to meet WCAG accessibility standards:
.disabledStep { - color: var(--disabled, #CDCCCC); + color: var(--disabled, #767676); cursor: not-allowed; }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
components/domains/steps/selectPfp.tsx (1)
Line range hint
31-35: Maintain architectural consistency with existing patterns.The
skipfunction properly cleans up form state before proceeding, but the close button doesn't follow this pattern. This inconsistency could lead to stale form state.Consider extracting the exit logic into a shared function:
const handleExit = (destination: string) => { if (selectedPfp && !window.confirm("Are you sure you want to exit? Your progress will be lost.")) { return; } updateFormState({ selectedPfp: undefined }); if (destination === "next") { goToNextStep(); } else { router.push(destination); } };Then use it in both places:
- const skip = () => { - updateFormState({ selectedPfp: undefined }); - goToNextStep(); - }; + const skip = () => handleExit("next"); // ... in JSX <button - onClick={() => router.push("/")} + onClick={() => handleExit("/")} >Also applies to: 51-57
components/domains/steps/registerSteps.tsx (2)
36-55: Consider reducing code duplication in loading state.The skeleton loading state has three identical blocks that could be made more maintainable.
Consider refactoring to reduce duplication:
- <div className={styles.stepsContainer} role="status" aria-label="Loading registration steps"> -<div className="flex gap-2 items-center"> - <Skeleton variant="circular" width={24} height={24} /> - <div className="w-12 sm:w-16 md:w-24 lg:w-36"> - <Skeleton variant="text" height={20} /> - </div> -</div> -<div className="flex gap-2 items-center"> - <Skeleton variant="circular" width={24} height={24} /> - <div className="w-12 sm:w-16 md:w-24 lg:w-36"> - <Skeleton variant="text" height={20} /> - </div> -</div> -<div className="flex gap-2 items-center"> - <Skeleton variant="circular" width={24} height={24} /> - <div className="w-12 sm:w-16 md:w-24 lg:w-36"> - <Skeleton variant="text" height={20} /> - </div> -</div> + <div className={styles.stepsContainer} role="status" aria-label="Loading registration steps"> + {[...Array(3)].map((_, index) => ( + <div key={index} className="flex gap-2 items-center"> + <Skeleton variant="circular" width={24} height={24} /> + <div className="w-12 sm:w-16 md:w-24 lg:w-36"> + <Skeleton variant="text" height={20} /> + </div> + </div> + ))} </div>
70-114: Standardize DoneFilledIcon color prop usage.There's an inconsistency in how the DoneFilledIcon component's color props are used:
- Some instances use both
secondColorandcolorprops- One instance uses only the
colorpropStandardize the usage:
- <DoneFilledIcon width="16" color={theme.palette.primary.main} /> + <DoneFilledIcon + width="16" + secondColor={theme.palette.primary.main} + color="#FFF" + />styles/components/registerV3.module.css (2)
251-251: Use CSS variable for border color.The border color uses a hex value with opacity (#4545451A) directly instead of a CSS variable.
Consider using a CSS variable for consistency:
- border: 1px solid #4545451A; + border: 1px solid var(--border-color-light);
Line range hint
449-519: Consider consolidating media queries.The media queries for max-width 400px and 350px have very similar styles. Consider consolidating them to reduce code duplication.
You could combine them like this:
-@media screen and (max-width: 400px) { +@media screen and (max-width: 400px) { .legend { - font-size: 10px; + font-size: clamp(9px, 2.5vw, 10px); letter-spacing: -0.3px; } .price { - font-size: 1rem; + font-size: clamp(0.8rem, 2.5vw, 1rem); } /* ... other styles ... */ } - -@media screen and (max-width: 350px) { - .legend { - font-size: 9px; - letter-spacing: -0.4px; - } - .price { - font-size: 0.8rem; - } - /* ... duplicate styles ... */ -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
components/domains/registerV3.tsx(3 hunks)components/domains/steps/registerSteps.tsx(1 hunks)components/domains/steps/selectPfp.tsx(2 hunks)hooks/useAllowanceCheck.tsx(1 hunks)styles/components/profilePic.module.css(1 hunks)styles/components/registerV3.module.css(10 hunks)styles/globals.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/domains/registerV3.tsx
🧰 Additional context used
📓 Learnings (1)
styles/components/registerV3.module.css (1)
Learnt from: Viktohblake
PR: lfglabs-dev/app.starknet.id#995
File: styles/components/registerV3.module.css:0-0
Timestamp: 2025-01-26T19:24:53.180Z
Learning: While maintaining design fidelity is important, accessibility requirements (like WCAG color contrast standards) should take precedence. When implementing designs from Figma, validate color contrast ratios and suggest accessible alternatives that maintain the design intent.
🔇 Additional comments (3)
styles/globals.css (1)
27-27: LGTM! The new color variable follows the established pattern.The new
--tertiary-600color variable (#ccb297) is appropriately positioned within the existing color scheme, maintaining consistency with other tertiary colors and following the established naming convention.Let's verify the color usage in the codebase:
✅ Verification successful
Color variable is properly defined and used consistently
The new
--tertiary-600color variable is being used appropriately in the registration components, with no duplicate or hardcoded instances found elsewhere in the codebase. The color scheme maintains a logical progression from dark to light: tertiary (#bf9e7b) → tertiary-600 (#ccb297) → light-tertiary (#f5f0eb).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check where the new color variable is being used # and ensure it's not duplicating any existing colors # Check for direct usage of the new variable rg "var\(--tertiary-600\)" --type css --type tsx --type jsx # Check for potential hardcoded usage of the same color value rg "#ccb297" --type css --type tsx --type jsx # Check for any other tertiary color variables to ensure consistent naming rg "tertiary.*:" --type cssLength of output: 331
Script:
#!/bin/bash # Retry with correct file type flags # Check for direct usage of the new variable rg "var\(--tertiary-600\)" -t css -t typescript -t javascriptreact -t typescriptreact # Check for potential hardcoded usage of the same color value rg "#ccb297" -t css -t typescript -t javascriptreact -t typescriptreact # List all files that might use this color variable fd "\.(css|tsx|jsx|ts|js)$"Length of output: 12019
Script:
#!/bin/bash # Final attempt with simplified search # Search for the new variable usage rg --type-add 'web:*.{css,tsx,jsx,ts,js}' -t web 'tertiary-600' # Search for hardcoded color value rg --type-add 'web:*.{css,tsx,jsx,ts,js}' -t web 'ccb297'Length of output: 456
components/domains/steps/registerSteps.tsx (1)
17-27: LGTM! Well-structured helper functions.The helper functions are well-designed with clear single responsibilities and consistent logic patterns.
styles/components/profilePic.module.css (1)
69-69: LGTM! Improved responsiveness.Changing from fixed width to 100% improves the component's adaptability to different screen sizes.
| <div className={styles.closeIcon}> | ||
| <button | ||
| onClick={() => router.push("/")} | ||
| > | ||
| <CloseIcon /> | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add confirmation dialog before navigation.
The close button implementation could lead to accidental data loss as it navigates away without warning. Users might accidentally click it and lose their form progress.
Consider this implementation:
<div className={styles.closeIcon}>
<button
- onClick={() => router.push("/")}
+ onClick={() => {
+ if (selectedPfp && !window.confirm("Are you sure you want to exit? Your progress will be lost.")) {
+ return;
+ }
+ updateFormState({ selectedPfp: undefined }); // Clean up form state
+ router.push("/");
+ }}
>
<CloseIcon />
</button>
</div>Committable suggestion skipped: line range outside the PR's diff.
hooks/useAllowanceCheck.tsx
Outdated
| @@ -34,7 +34,7 @@ export default function useAllowanceCheck( | |||
| erc20AllowanceRes["remaining"].high !== UINT_128_MAX) | |||
| ) { | |||
| setNeedsAllowance(true); | |||
| setCurrentAllowance(BigInt(erc20AllowanceRes["remaining"].low)); | |||
| // setCurrentAllowance(BigInt(erc20AllowanceRes["remaining"].low)); | |||
There was a problem hiding this comment.
💡 Codebase verification
Restore allowance tracking to prevent unnecessary token approvals
The commented line affects user experience and gas costs but doesn't pose a security risk. By not tracking the current allowance:
- Users may need to approve full amounts even when they have sufficient partial allowances
- This results in unnecessary gas costs for users
- Auto-renewal and registration flows still work but are less efficient
- // setCurrentAllowance(BigInt(erc20AllowanceRes["remaining"].low));
+ setCurrentAllowance(BigInt(erc20AllowanceRes["remaining"].low));🔗 Analysis chain
Critical: Restore allowance checking logic or justify its removal.
Commenting out the setCurrentAllowance line has several concerning implications:
- The UI will always show 0 allowance when
needsAllowanceis true, even if there's a partial allowance. - This could force users to approve the full amount again, leading to unnecessary gas costs.
- The commented code should be either restored or removed entirely, not left as a comment.
If this change is intentional, please:
- Remove the commented line instead of leaving it in the code
- Add a comment explaining why we're ignoring partial allowances
- Update the function's documentation to reflect this behavior change
Otherwise, restore the original logic with this diff:
- // setCurrentAllowance(BigInt(erc20AllowanceRes["remaining"].low));
+ setCurrentAllowance(BigInt(erc20AllowanceRes["remaining"].low));Let's verify the impact of this change on the registration flow:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components that depend on currentAllowance
echo "Components using currentAllowance:"
rg -l "useAllowanceCheck" | while read -r file; do
echo "=== $file ==="
rg "currentAllowance" "$file" -A 5
done
# Search for any error handling or validation logic related to allowances
echo -e "\nAllowance-related error handling:"
rg -i "allowance.*error|insufficient.*allowance" -A 5
Length of output: 3280
Script:
#!/bin/bash
# Show how currentAllowance is used in dependent components
echo "=== Checking currentAllowance usage in registerDiscount.tsx ==="
rg -A 10 "const \{.*currentAllowance" "components/discount/registerDiscount.tsx" || echo "No direct usage found"
echo -e "\n=== Checking currentAllowance usage in checkoutCard.tsx ==="
rg -A 10 "const \{.*currentAllowance" "components/domains/steps/checkoutCard.tsx" || echo "No direct usage found"
# Look for UI elements that might display allowance values
echo -e "\n=== Checking for allowance display in UI ==="
rg -i "allowance.*\$|currentAllowance.*\$" -A 5
Length of output: 95491
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
components/domains/registerV3.tsx (3)
59-72: Enhance accessibility of the navigation section.While the navigation role is correctly applied, adding an aria-label would improve screen reader context.
- <aside className={`${styles.purchaseStepNav}`} role="navigation"> + <aside + className={`${styles.purchaseStepNav}`} + role="navigation" + aria-label="Domain purchase steps navigation" + >
74-88: Apply consistent accessibility attributes to mobile navigation.The mobile navigation should have the same accessibility enhancements as the desktop version.
- <div className={`${styles.purchaseStepNavMobile}`} role="navigation"> + <div + className={`${styles.purchaseStepNavMobile}`} + role="navigation" + aria-label="Domain purchase steps navigation" + >
90-104: Add aria-live region for dynamic content.The step content area changes dynamically and should be marked as a live region for screen readers.
- <div className="flex-1"> + <div + className="flex-1" + aria-live="polite" + role="region" + aria-label="Registration step content" + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/domains/registerV3.tsx(2 hunks)
🧰 Additional context used
📓 Learnings (1)
components/domains/registerV3.tsx (1)
Learnt from: Viktohblake
PR: lfglabs-dev/app.starknet.id#995
File: components/domains/registerV3.tsx:19-21
Timestamp: 2025-01-26T19:18:53.841Z
Learning: In the `RegisterV3` component (`components/domains/registerV3.tsx`), the `setDomain` prop is kept despite being unused because removing it would require changes to dependent files. This prop is marked with a comment indicating it's not used since the domain search bar was removed.
🔇 Additional comments (1)
components/domains/registerV3.tsx (1)
Line range hint
29-39: Consider adding missing dependencies to useEffect.The effect initializes form state but only depends on
domain. Consider addingupdateFormStateto the dependency array and wrapping it with useCallback in the context to prevent unnecessary re-renders.
|
Also a bit more margin here on mobile would be good |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
styles/components/walletConnect.module.css (1)
Line range hint
139-152: Add mobile-specific margins per reviewer feedback.Based on Marchand-Nicolas's feedback about needing "more margin on mobile", consider adding margin adjustments to the mobile breakpoint:
@media (max-width: 1024px) { .menu { width: 320px; padding: 30px; } .modalTitle { margin: -7px -2px 2px; } + + .connectBtn { + margin: 0 12px; + } }
🧹 Nitpick comments (1)
styles/components/walletConnect.module.css (1)
114-114: Consider adding overflow protection.While
white-space: nowrapis good for preventing awkward button text wrapping, consider adding overflow protection:.connectBtn { display: flex; flex-direction: row; align-items: center; justify-content: space-between; gap: 8px; white-space: nowrap; + max-width: 100%; + overflow: hidden; + text-overflow: ellipsis; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
styles/components/profilePic.module.css(2 hunks)styles/components/walletConnect.module.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- styles/components/profilePic.module.css
There was a problem hiding this comment.
To much space here, it should be like on figma
There is too much margin here on mobile, please put the same as on figma
(Also the cancel button is missing in the first step on mobile)
Too much margin here (see on figma)
Lastly, on desktop, the "X" cross to close the page is at a different position at every step, can you make it static please ? (You can use absolute position, or even fixed)
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
components/domains/steps/userInfoForm.tsx (1)
Line range hint
92-94: Add confirmation dialog for unsaved changes.The
handleClosefunction immediately navigates away, potentially causing users to lose their form data. Consider adding a confirmation dialog when there are unsaved changes.const handleClose = () => { // Check if form has unsaved changes const hasUnsavedChanges = formState.durationInYears > 0 || formState.tokenId; if (hasUnsavedChanges) { const confirmed = window.confirm('You have unsaved changes. Are you sure you want to leave?'); if (!confirmed) return; } router.push('/'); };components/domains/steps/checkoutCard.tsx (1)
Line range hint
313-318: Add error handling for transaction execution.The transaction execution lacks error handling, which could leave users in a stuck state if the transaction fails.
Consider adding error handling:
<Button onClick={() => execute().then(() => { setDomainsMinting(formState.selectedDomains); - }) + }) + .catch((error) => { + console.error('Transaction failed:', error); + // Show error notification to user + addTransaction({ + timestamp: Date.now(), + subtext: 'Transaction failed', + type: NotificationType.ERROR, + data: { error: error.message }, + }); + }) } disabled={
🧹 Nitpick comments (3)
components/domains/steps/userInfoForm.tsx (1)
Line range hint
92-94: Consider consolidating duplicate cancel actions.The component currently has two ways to cancel/close:
- Close icon at the top (
handleClose)- Cancel button in the summary section (same
handleClose)Consider consolidating these into a single, clear call-to-action to improve UX and maintainability.
Options:
- Keep only the cancel button in the summary section (more conventional placement)
- Keep only the close icon (if following a modal-like pattern)
- If keeping both, consider renaming one for clarity (e.g., "Close" vs "Cancel")
Also applies to: 159-162
components/domains/steps/checkoutCard.tsx (1)
Line range hint
334-339: Improve close button accessibility and code organization.Two suggestions for improvement:
- Add an aria-label for screen readers
- Consider extracting the navigation logic to avoid duplication
+ const handleClose = useCallback(() => router.push("/"), [router]); + return ( // ... <div className={styles.closeIcon}> <button - onClick={() => router.push("/")} + onClick={handleClose} + aria-label="Close checkout and return to home" > <CloseIcon /> </button> </div>Then reuse
handleClosein the cancel button:<div className={styles.cancelBtn} - onClick={() => router.push("/")} + onClick={handleClose} >components/domains/steps/registerSteps.tsx (1)
58-114: Enhance step navigation accessibility.While the structure is good, consider adding keyboard navigation support for better accessibility.
<div className={styles.stepsContainer} aria-label="Registration steps"> + <div role="tablist"> <div className={`${styles.step} ${getStep(currentStep, 1)}`} + role="tab" + tabIndex={currentStep >= 1 ? 0 : -1} + aria-selected={currentStep === 1} onClick={() => currentStep >= 1 && setStep(1)} + onKeyPress={(e) => e.key === 'Enter' && currentStep >= 1 && setStep(1)} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
components/domains/registerV3.tsx(2 hunks)components/domains/steps/checkoutCard.tsx(2 hunks)components/domains/steps/registerSteps.tsx(1 hunks)components/domains/steps/userInfoForm.tsx(1 hunks)styles/components/button.module.css(1 hunks)styles/components/profilePic.module.css(3 hunks)styles/components/registerV3.module.css(11 hunks)styles/components/upsellCard.module.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- components/domains/registerV3.tsx
- styles/components/button.module.css
- styles/components/profilePic.module.css
🧰 Additional context used
📓 Learnings (1)
styles/components/registerV3.module.css (1)
Learnt from: Viktohblake
PR: lfglabs-dev/app.starknet.id#995
File: styles/components/registerV3.module.css:0-0
Timestamp: 2025-01-26T19:24:53.180Z
Learning: While maintaining design fidelity is important, accessibility requirements (like WCAG color contrast standards) should take precedence. When implementing designs from Figma, validate color contrast ratios and suggest accessible alternatives that maintain the design intent.
🔇 Additional comments (7)
components/domains/steps/checkoutCard.tsx (1)
303-303: LGTM! Improved button spacing in invalid states.The conditional class name adjustment helps maintain a more compact layout when validation messages are shown.
styles/components/upsellCard.module.css (2)
77-86: LGTM! Responsive design improvements for larger screens.The new media query for 1280px breakpoint appropriately handles text alignment for better readability.
91-91: LGTM! Container width adjustment.The width property ensures the container takes full width while max-width prevents overflow.
components/domains/steps/registerSteps.tsx (2)
17-27: LGTM! Well-structured helper functions.The
getStepandgetStepColorfunctions improve code maintainability and readability by centralizing style logic.
36-55: LGTM! Accessible loading state implementation.The loading state now includes proper ARIA attributes as suggested in previous reviews.
styles/components/registerV3.module.css (2)
254-283: LGTM! Responsive step navigation implementation.The step navigation appropriately switches between desktop and mobile views using display properties.
226-226: Use CSS variable names consistently.As mentioned in past reviews, use only variable names without fallback colors:
- background: var(--tertiary-600); + background: var(--tertiary); - color: var(--secondary); + background: var(--secondary);Also applies to: 315-315
Close #950
Summary by CodeRabbit
Release Notes
UI/Icon Updates
Registration Flow Improvements
Styling Enhancements