-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Restart 1 #4
base: main
Are you sure you want to change the base?
Restart 1 #4
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Reviewer's Guide by SourceryThis pull request significantly refactors the main page by simplifying its structure and removing several components. New UI components and utilities have been added to enhance the user experience, including a new Navbar and LogoBehind component. Global styles have been updated, and deprecated components have been removed. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @sam-the-programmer - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
src/lib/index.ts
Outdated
mediaQuery.addEventListener("change", (event) => { | ||
reduceMotion = event.matches; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): Potential issue with event listener.
Consider removing the event listener when the component is unmounted to avoid potential memory leaks.
mediaQuery.addEventListener("change", (event) => { | |
reduceMotion = event.matches; | |
const handleChange = (event) => { | |
reduceMotion = event.matches; | |
}; | |
mediaQuery.addEventListener("change", handleChange); | |
return () => { | |
mediaQuery.removeEventListener("change", handleChange); | |
}; |
@@ -0,0 +1,6 @@ | |||
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 32 32" width="32" height="32" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Missing ARIA attributes for SVG.
Consider adding ARIA attributes to the SVG element to improve accessibility, such as role="img"
and aria-label
.
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 32 32" width="32" height="32" | |
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 32 32" width="32" height="32" role="img" aria-label="Right Arrow"> | |
<path | |
d="m31.71 15.29-10-10-1.42 1.42 8.3 8.29H0v2h28.59l-8.29 8.29 1.41 1.41 10-10a1 1 0 0 0 0-1.41z" | |
data-name="3-Arrow Right" | |
/> | |
</svg> |
Summary by Sourcery
This pull request refactors the main page layout, introduces new UI components for logo and navigation, and removes obsolete components. It also updates global styles and integrates performance monitoring.
LogoBehind
component to display a background logo with animations.Navbar
component with a responsive design and a hamburger menu for mobile view.RightArrow
icon component for navigation purposes.app.postcss
to improve consistency and readability.Navbar.svelte
,LandingText.svelte
,Footer.svelte
,Navlinks.svelte
, andCircles.svelte
.