feat: add home UI #34
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces multiple enhancements across several components, including the addition of new icon components, a new newsletter form, and a layout component. The Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 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
Documentation and Community
|
⚡️ Lighthouse ReportMobile (view report)
Desktop (view report)
Lighthouse ran on https://uniswap-docs-staging-f9gd8t93l-uniswap.vercel.app |
limsohee1002
force-pushed
the
feat/home
branch
from
88f5ac5 to
99bd536
Compare
⚡️ Lighthouse ReportMobile (view report)
Desktop (view report)
Lighthouse ran on https://uniswap-docs-staging-2y24n5erb-uniswap.vercel.app |
limsohee1002
force-pushed
the
feat/home
branch
from
99bd536 to
d0c0d3b
Compare
⚡️ Lighthouse ReportMobile (view report)
Desktop (view report)
Lighthouse ran on https://uniswap-docs-staging-klqmq5zdn-uniswap.vercel.app |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (7)
src/theme/Layout.tsx (2)
9-18: Consider enhancing component resilience and accessibilityWhile the basic layout structure is good, consider the following improvements:
- Add error boundaries to handle potential rendering failures
- Include ARIA landmarks for better accessibility
- Consider moving the hardcoded background colors to theme variables
Here's a suggested improvement:
-const Layout: FC<Props> = ({ title, description, children }) => { +const Layout: FC<Props> = ({ title, description, children }) => { return ( <LayoutProvider> <PageMetadata title={title} description={description} /> - <Navbar /> + <header role="banner"> + <Navbar /> + </header> <main className="pt-nav-h min-h-screen flex flex-row bg-light-surface-1 dark:bg-dark-surface-1">{children}</main> - <Footer /> + <footer role="contentinfo"> + <Footer /> + </footer> </LayoutProvider> ); }
14-14: Consider decomposing className for better maintainabilityThe main content className combines multiple styling concerns. Consider extracting these into separate utility classes or CSS modules for better maintainability.
-<main className="pt-nav-h min-h-screen flex flex-row bg-light-surface-1 dark:bg-dark-surface-1">{children}</main> +<main className={clsx( + 'pt-nav-h', + 'min-h-screen', + 'flex flex-row', + 'bg-light-surface-1 dark:bg-dark-surface-1' +)}>{children}</main>src/css/custom.css (1)
26-28: LGTM! Consider adding a comment describing breakpoint behavior.The responsive padding implementation follows Tailwind's mobile-first approach and provides appropriate spacing across different viewport sizes.
Add a comment to document the padding values at different breakpoints:
+/* Responsive padding: + * - Mobile: p-6 pb-10 + * - SM (640px+): p-12 pt-6 + * - MD (768px+): px-10 pb-12 + */ .content-page-padding { @apply p-6 pb-10 sm:p-12 sm:pt-6 md:px-10 md:pb-12; }src/theme/Navbar.tsx (2)
11-12: Consider a more robust solution for Docusaurus navbar class requirementWhile the hidden div works as a workaround, it might be fragile. Consider:
- Adding the "navbar" class to the actual nav element
- Documenting the specific Docusaurus version and issue number in the comment
- {/* docusaurus expects class name "navbar". adding fake div for handling error that is looking for "navbar" class */} - <div className="navbar hidden"></div> - <nav className="Navbar fixed left-0 right-0 z-nav flex w-screen justify-center bg-light-surface-1 dark:bg-dark-surface-1 dark:border-dark-surface-3"> + {/* Docusaurus v2.x.x requires "navbar" class for theme compatibility (#issue-number) */} + <nav className="navbar Navbar fixed left-0 right-0 z-nav flex w-screen justify-center bg-light-surface-1 dark:bg-dark-surface-1 dark:border-dark-surface-3">
15-18: Remove redundant target="_self" attributeThe Link implementation is good, with proper accessibility attributes. However,
target="_self"is redundant as it's the default behavior for internal links.- <Link className="flex flex-row items-center" to="/" target="_self" aria-label="Uniswap Documentation Home"> + <Link className="flex flex-row items-center" to="/" aria-label="Uniswap Documentation Home">src/components/NewsletterForm/index.tsx (1)
52-86: Enhance form accessibility and user experienceWhile the form implementation is good, it needs some accessibility and UX improvements:
<form onSubmit={handleSubscribe}> <div className="flex flex-col sm:flex-row"> <div className="relative w-full"> + <label htmlFor="email" className="sr-only">Email Address</label> <input + id="email" className={cn('button-label-2 w-full flex-grow rounded-large p-3 pl-[2.7rem]', inputClass)} - aria-label="Enter Email" type="email" + required + pattern="[^\s@]+@[^\s@]+\.[^\s@]+" placeholder="Enter Email" value={emailInputValue} onChange={(e) => setEmailInputValue(e.target.value)} /> - <Envlope className="absolute left-3 top-[50%] h-6 w-6 translate-y-[-50%]" color={globeColorClass} /> + <Envelope className="absolute left-3 top-[50%] h-6 w-6 translate-y-[-50%]" color={globeColorClass} /> </div> - <button className="group mt-2 flex items-center justify-center rounded-large bg-light-pink-vibrant p-3 w-32 transition hover:bg-dark-accent-2 dark:bg-dark-pink-vibrant hover:dark:bg-light-accent-2 sm:ml-2 sm:mt-0"> + <button + type="submit" + disabled={isLoading} + className={cn( + "group mt-2 flex items-center justify-center rounded-large p-3 w-32 transition sm:ml-2 sm:mt-0", + isLoading + ? "bg-gray-300 cursor-not-allowed" + : "bg-light-pink-vibrant hover:bg-dark-accent-2 dark:bg-dark-pink-vibrant hover:dark:bg-light-accent-2" + )}> <span className="button-label-1 text-white transition group-hover:text-dark-accent-1 group-hover:dark:text-light-accent-1"> - Submit + {isLoading ? 'Submitting...' : 'Submit'} </span> </button>src/components/Icons/index.tsx (1)
335-352: Make Npm icon colors theme-awareThe component uses hardcoded colors which don't respect the theme system. Consider making it consistent with other icons by using theme-based colors.
-export const Npm: FC<{ className?: string }> = ({ className }) => { +export const Npm: FC<{ className?: string; color?: 'accent-1' }> = ({ className, color = 'accent-1' }) => { return ( <svg className={className} width="256px" height="256px" viewBox="0 0 256 256" version="1.1" xmlns="http://www.w3.org/2000/svg" preserveAspectRatio="xMidYMid" > <g> - <polygon fill="#C12127" points="0 256 0 0 256 0 256 256"></polygon> - <polygon fill="#FFFFFF" points="48 48 208 48 208 208 176 208 176 80 128 80 128 208 48 208"></polygon> + <polygon className={cn({ + 'fill-light-accent-1 dark:fill-dark-accent-1': color === 'accent-1', + })} points="0 256 0 0 256 0 256 256"></polygon> + <polygon className="fill-white dark:fill-white" points="48 48 208 48 208 208 176 208 176 80 128 80 128 208 48 208"></polygon> </g> </svg> ) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (6)
static/img/Union.svgis excluded by!**/*.svgstatic/img/logo.svgis excluded by!**/*.svgstatic/img/undraw_docusaurus_mountain.svgis excluded by!**/*.svgstatic/img/undraw_docusaurus_react.svgis excluded by!**/*.svgstatic/img/undraw_docusaurus_tree.svgis excluded by!**/*.svgstatic/img/uni_dark_icon.svgis excluded by!**/*.svg
📒 Files selected for processing (7)
src/components/Icons/index.tsx(4 hunks)src/components/NewsletterForm/index.tsx(1 hunks)src/css/custom.css(1 hunks)src/pages/index.tsx(2 hunks)src/theme/Layout.tsx(1 hunks)src/theme/Navbar.tsx(1 hunks)tailwind.config.js(1 hunks)
🔇 Additional comments (10)
src/theme/Layout.tsx (2)
1-8: LGTM! Clean and well-organized imports.
The imports are properly structured with React imports first, followed by theme-specific imports, and type imports are appropriately handled.
1-20: Verify integration with NewsletterForm component
The AI summary mentions a new NewsletterForm component, but it's not clear how it integrates with this Layout component.
src/css/custom.css (2)
30-32: LGTM! Clean and theme-aware implementation.
The divider implementation correctly uses theme-specific color tokens and handles both light and dark modes appropriately.
26-32: Verify consistent usage across components.
Let's ensure these utility classes are being used consistently across components and that there aren't multiple implementations of similar styles.
✅ Verification successful
Let me gather more information about the usage of these classes by trying different file extensions and using a more comprehensive search approach.
CSS utility classes are properly implemented with no duplication
The verification shows that:
- Both utility classes are used exclusively in
src/pages/index.tsx - No duplicate or similar padding patterns exist in other CSS files
- No duplicate or similar border patterns were found
- The styles are centralized in
custom.cssand used consistently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of the new utility classes and potential duplicate implementations
# Check where these classes are being used
echo "Checking usage of content-page-padding and divider classes:"
rg -l "content-page-padding|divider" --type html --type jsx --type tsx
# Look for potential duplicate implementations
echo "\nChecking for potential duplicate padding patterns:"
rg -B1 -A1 "@apply.*p-6.*pb-10.*sm:p-12" --type css
echo "\nChecking for potential duplicate divider patterns:"
rg -B1 -A1 "@apply.*border-t.*border-light-surface-3" --type css
Length of output: 856
Script:
#!/bin/bash
# Search for usage of the classes across different file types
echo "Searching for content-page-padding usage:"
rg "content-page-padding" --type-add 'web:*.{js,jsx,ts,tsx,vue,html}' --type web -l
echo "\nSearching for divider class usage:"
rg "divider" --type-add 'web:*.{js,jsx,ts,tsx,vue,html}' --type web -l
# Look for similar padding patterns that might indicate inconsistent styling
echo "\nChecking for similar padding patterns:"
rg "@apply.*p-6|@apply.*sm:p-12|@apply.*md:px-10" --type css
# Look for similar border patterns
echo "\nChecking for similar border patterns:"
rg "@apply.*border-t.*border-.*surface-3|@apply.*border.*surface-3" --type css
Length of output: 855
src/theme/Navbar.tsx (2)
3-3: LGTM! Correct usage of Docusaurus Link component
Using @docusaurus/Link instead of HTML anchor tags is the recommended approach for navigation in Docusaurus.
30-40: LGTM! Well-implemented external link with proper security attributes
The feedback link implementation follows best practices:
- Uses proper security attributes (
rel="noreferrer") - Includes proper target attribute for external link
- Implements proper dark/light mode styling
src/components/NewsletterForm/index.tsx (2)
12-21: LGTM! Well-structured component with proper TypeScript typing
The component definition and state management follow React best practices with appropriate use of TypeScript types and useState hooks.
1-89: Verify for duplicate newsletter implementations
Let's ensure this is the only newsletter form implementation to maintain consistency across the codebase.
✅ Verification successful
Newsletter form implementation is consistent
The verification confirms this is the only newsletter form implementation in the codebase, properly used in the homepage (index.tsx). No duplicate implementations or inconsistent form submission patterns were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential newsletter form implementations
echo "Searching for other newsletter form implementations..."
rg -l "newsletter|subscribe|email.*input" --type tsx --type jsx
# Search for similar API endpoint usage
echo "Searching for similar API endpoint usage..."
rg -l "fetch.*api.*subscribe|fetch.*newsletter" --type tsx --type jsx
Length of output: 409
Script:
#!/bin/bash
# Search for other potential newsletter form implementations with correct file extensions
echo "Searching for other newsletter form implementations..."
rg -l "newsletter|subscribe|email.*input" -g "*.ts" -g "*.tsx" -g "*.js" -g "*.jsx"
# Search for similar API endpoint usage
echo "Searching for similar API endpoint usage..."
rg -l "fetch.*api.*subscribe|fetch.*newsletter" -g "*.ts" -g "*.tsx" -g "*.js" -g "*.jsx"
# Search for components with similar form submission patterns
echo "Searching for components with similar form patterns..."
ast-grep --pattern 'const $_ = async (e: React.FormEvent) => { e.preventDefault(); $$$}'
Length of output: 663
Script:
#!/bin/bash
# Let's check the content of index.tsx to see how the NewsletterForm is used
echo "Checking index.tsx content..."
rg -A 5 -B 5 "NewsletterForm" src/pages/index.tsx
# Let's also verify if there are any other form submission handlers
echo "Checking for other form submission patterns..."
rg "onSubmit=|handleSubmit|submitForm" -g "*.ts" -g "*.tsx" -g "*.js" -g "*.jsx"
Length of output: 1405
tailwind.config.js (1)
162-168: Verify color contrast ratios for accessibility.
Please ensure that the new color combinations meet WCAG accessibility guidelines for contrast ratios, particularly for text and interactive elements.
Consider using a color contrast checker tool in your development workflow to ensure all color combinations meet WCAG 2.1 Level AA standards (minimum contrast ratio of 4.5:1 for normal text and 3:1 for large text).
src/components/Icons/index.tsx (1)
179-225: LGTM!
The Emblem components are well-implemented with consistent patterns and proper TypeScript types.
src/components/Icons/index.tsx
Outdated
| export const Envlope: FC<{ | ||
| className?: string | ||
| color?: 'pink-vibrant' | 'neutral-2' | ||
| }> = ({ className, color = 'pink-vibrant' }) => { | ||
| return ( | ||
| <svg className={className} viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg"> | ||
| <path | ||
| d="M18 5H6C4 5 3 6 3 8V17C3 19 4 20 6 20H18C20 20 21 19 21 17V8C21 6 20 5 18 5ZM17.9409 9.606L13.0291 13.178C12.7211 13.402 12.36 13.514 12 13.514C11.64 13.514 11.2779 13.402 10.9709 13.179L6.05908 9.606C5.72408 9.363 5.65004 8.893 5.89404 8.558C6.13704 8.224 6.60389 8.14801 6.94189 8.39301L11.854 11.965C11.942 12.028 12.059 12.029 12.147 11.965L17.0591 8.39301C17.3961 8.14801 17.8639 8.224 18.1069 8.558C18.3509 8.894 18.2759 9.363 17.9409 9.606Z" | ||
| className={cn({ | ||
| 'fill-light-pink-vibrant dark:fill-dark-pink-vibrant': color === 'pink-vibrant', | ||
| 'fill-light-neutral-2 dark:fill-dark-neutral-2': color === 'neutral-2', | ||
| })} | ||
| /> | ||
| </svg> | ||
| ) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Fix typo in component name 'Envlope' and update its references
The component name has a typo that needs to be corrected in the following locations:
src/components/Icons/index.tsx: Component definitionsrc/components/NewsletterForm/index.tsx: Import statement and component usage (2 instances)
🔗 Analysis chain
Fix typo in component name
The component name 'Envlope' has a typo and should be 'Envelope'.
-export const Envlope: FC<{
+export const Envelope: FC<{
className?: string
color?: 'pink-vibrant' | 'neutral-2'
}> = ({ className, color = 'pink-vibrant' }) => {Make sure to update all references to this component in other files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all references to the misspelled component name
rg "Envlope" --type ts --type tsx
Length of output: 61
Script:
#!/bin/bash
# Find all references to the misspelled component name using ripgrep without file type filters
rg "Envlope"
Length of output: 312
⚡️ Lighthouse ReportMobile (view report)
Desktop (view report)
Lighthouse ran on https://uniswap-docs-staging-n1dwf14be-uniswap.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/Icons/index.tsx (2)
42-62: Consider expanding color options for better reusabilityThe component currently only supports 'green' color. Consider adding more color options to match the theme system used in other icon components.
-export const Hexagon: FC<{ className?: string; color?: 'green' }> = ({ className, color = 'green' }) => { +export const Hexagon: FC<{ + className?: string; + color?: 'green' | 'orange-vibrant' | 'blue-vibrant' | 'pink-vibrant' +}> = ({ className, color = 'green' }) => {
84-100: Consider standardizing the SVG viewBox sizeThe component uses a non-standard viewBox size (20x28) which might cause layout inconsistencies when used alongside other icons. Consider standardizing to 24x24 or 16x16 to match other icons.
- <svg className={className} viewBox="0 0 20 28" fill="none" xmlns="http://www.w3.org/2000/svg"> + <svg className={className} viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg">
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Icons/index.tsx(4 hunks)src/components/NewsletterForm/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/NewsletterForm/index.tsx
🔇 Additional comments (3)
src/components/Icons/index.tsx (3)
5-21: Previous review comment is still applicable
The component still has a hardcoded fill color which breaks theme consistency.
23-40: LGTM! Well-structured component with proper theme support
The component follows best practices with type-safe color options and proper theme class application.
64-82: LGTM! Consistent implementation
The component follows the established pattern with proper theme support.
| export const Npm: FC<{ className?: string }> = ({ className }) => { | ||
| return ( | ||
| <svg | ||
| className={className} | ||
| width="256px" | ||
| height="256px" | ||
| viewBox="0 0 256 256" | ||
| version="1.1" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| preserveAspectRatio="xMidYMid" | ||
| > | ||
| <g> | ||
| <polygon fill="#C12127" points="0 256 0 0 256 0 256 256"></polygon> | ||
| <polygon fill="#FFFFFF" points="48 48 208 48 208 208 176 208 176 80 128 80 128 208 48 208"></polygon> | ||
| </g> | ||
| </svg> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make Npm component consistent with other icons
The Npm component breaks the established pattern by using hardcoded colors. Consider making it consistent with other icons by adding theme support.
-export const Npm: FC<{ className?: string }> = ({ className }) => {
+export const Npm: FC<{
+ className?: string
+ color?: 'accent-1' | 'neutral-2'
+}> = ({ className, color = 'accent-1' }) => {
return (
<svg
className={className}
width="256px"
height="256px"
viewBox="0 0 256 256"
version="1.1"
xmlns="http://www.w3.org/2000/svg"
preserveAspectRatio="xMidYMid"
>
<g>
- <polygon fill="#C12127" points="0 256 0 0 256 0 256 256"></polygon>
- <polygon fill="#FFFFFF" points="48 48 208 48 208 208 176 208 176 80 128 80 128 208 48 208"></polygon>
+ <polygon className={cn({
+ 'fill-light-accent-1 dark:fill-dark-accent-1': color === 'accent-1',
+ 'fill-light-neutral-2 dark:fill-dark-neutral-2': color === 'neutral-2',
+ })} points="0 256 0 0 256 0 256 256"></polygon>
+ <polygon fill="currentColor" points="48 48 208 48 208 208 176 208 176 80 128 80 128 208 48 208"></polygon>
</g>
</svg>
)
}Committable suggestion skipped: line range outside the PR's diff.
⚡️ Lighthouse ReportMobile (view report)
Desktop (view report)
Lighthouse ran on https://uniswap-docs-staging-lrx52ayr6-uniswap.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tailwind.config.js (1)
162-168: Group related colors together for better maintainability.The new color definitions are added at the end of the colors object. Consider organizing them with similar existing colors:
- Move blue-related colors near other blue variations
- Move green-related colors near other green variations
Here's a suggested organization:
colors: { // ... other colors ... // Blue colors group 'dark-blue': '#10143D', 'light-blue': '#EFF4FF', 'blue-base': '#4981FF', // Green colors group 'dark-green': '#0F2C1A', 'light-green': '#EEFBF1', 'green-base': '#0C8911', 'green-vibrant': '#21C95E', // ... remaining colors ... }src/components/Icons/index.tsx (2)
440-454: Consider optimizing SVG path dataThe SVG path data could be optimized to reduce file size and improve rendering performance. Consider using an SVG optimizer tool to simplify the path data while maintaining visual quality.
Line range hint
1-590: Consider standardizing icon component structureTo improve maintainability and consistency, consider:
- Creating a base icon component with common props and SVG attributes
- Implementing a consistent color system across all icons
- Documenting the color theme options and their use cases
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/components/Icons/index.tsx(6 hunks)src/css/custom.css(1 hunks)tailwind.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/css/custom.css
🔇 Additional comments (3)
src/components/Icons/index.tsx (3)
335-352: Make component consistent with other icons
The component uses hardcoded colors (#C12127 and #FFFFFF) which breaks theme consistency.
503-518: LGTM!
The component follows best practices with proper color theming and consistent implementation.
5-21: 🛠️ Refactor suggestion
Make color configurable for consistency
The component has a hardcoded fill color (#4981FF) which breaks consistency with other icon components that support theming through color props.
Apply this diff to make it consistent:
-export const ThickX: FC<{ className?: string }> = ({ className }) => {
+export const ThickX: FC<{
+ className?: string
+ color?: 'accent-1'
+}> = ({ className, color = 'accent-1' }) => {
return (
<svg
className={className}
width="24"
height="24"
viewBox="0 0 24 24"
fill="none"
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M7.24264 2.48528C5.92893 1.17157 3.79899 1.17157 2.48528 2.48528C1.17157 3.79899 1.17157 5.92893 2.48528 7.24264L6.0533 10.8107C6.71016 11.4675 6.71016 12.5325 6.0533 13.1893L2.48528 16.7574C1.17157 18.0711 1.17157 20.201 2.48528 21.5147C3.79899 22.8284 5.92893 22.8284 7.24264 21.5147L10.8107 17.9467C11.4675 17.2898 12.5325 17.2898 13.1893 17.9467L16.7574 21.5147C18.0711 22.8284 20.201 22.8284 21.5147 21.5147C22.8284 20.201 22.8284 18.0711 21.5147 16.7574L17.9467 13.1893C17.2898 12.5325 17.2898 11.4675 17.9467 10.8107L21.5147 7.24264C22.8284 5.92893 22.8284 3.79899 21.5147 2.48528C20.201 1.17157 18.0711 1.17157 16.7574 2.48528L13.1893 6.0533C12.5325 6.71016 11.4675 6.71016 10.8107 6.0533L7.24264 2.48528Z"
- fill="#4981FF"
+ className={cn({
+ 'fill-light-accent-1 dark:fill-dark-accent-1': color === 'accent-1',
+ })}
/>
</svg>
)
}Likely invalid or redundant comment.
⚡️ Lighthouse ReportMobile (view report)
Desktop (view report)
Lighthouse ran on https://uniswap-docs-staging-2swxzzb7h-uniswap.vercel.app |
Description
add home UI
Type(s) of changes
Motivation for PR
How Has This Been Tested?
Applicable screenshots
Follow-up PR
Summary by CodeRabbit
Release Notes
New Features
NewsletterFormcomponent for email subscriptions with error handling.Layoutcomponent for consistent page structure.Improvements
Styling