Uh oh!
There was an error while loading. Please reload this page.
Replace old hero with minimal star animations - #1122
Conversation
@NiallJoeMaher is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
Poem
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 ( |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (5)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonpackage.jsonis excluded by!**/*.jsonpublic/images/home/moon.pngis excluded by!**/*.png,!**/*.pngpublic/images/home/rocketman.pngis excluded by!**/*.png,!**/*.pngpublic/images/home/space.jpgis excluded by!**/*.jpg,!**/*.jpg
📒 Files selected for processing (1)
- components/Hero/Hero.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
components/Hero/Hero.tsx (2)
5-39: CoduLogo Component Implementation is CorrectThe
CoduLogocomponent correctly renders the SVG logo with appropriate accessibility attributes, such asaria-labelandrole="img".
211-236: Review the Use ofaria-hidden="true"onCoduLogoAt line 221, the
CoduLogocomponent is used witharia-hidden="true". Since the SVG logo includesaria-labelandrole="img"for accessibility, settingaria-hidden="true"will hide it from assistive technologies, potentially affecting users who rely on screen readers.Please verify whether the logo is purely decorative. If it conveys meaningful information or branding important to the user experience, consider removing
aria-hidden="true"to allow assistive technologies to announce it.
| const styles = ` | ||
| @keyframes twinkle { | ||
| 0%, 100% { opacity: 0.3; } | ||
| 50% { opacity: 0.7; } | ||
| } | ||
| @keyframes gentleMove1 { | ||
| 0%, 100% { transform: translate(0, 0); } | ||
| 50% { transform: translate(3px, 2px); } | ||
| } | ||
| @keyframes gentleMove2 { | ||
| 0%, 100% { transform: translate(0, 0); } | ||
| 50% { transform: translate(-2px, 4px); } | ||
| } | ||
| @keyframes gentleMove3 { | ||
| 0%, 100% { transform: translate(0, 0); } | ||
| 50% { transform: translate(4px, -3px); } | ||
| } | ||
| @keyframes shootingStar { | ||
| 0% { transform: translate(0, 0); opacity: 1; } | ||
| 100% { transform: translate(300px, 300px); opacity: 0; } | ||
| } | ||
| .twinkle { animation: twinkle 4s ease-in-out infinite; } | ||
| .gentle-move1 { animation: gentleMove1 25s ease-in-out infinite; } | ||
| .gentle-move2 { animation: gentleMove2 30s ease-in-out infinite; } | ||
| .gentle-move3 { animation: gentleMove3 35s ease-in-out infinite; } | ||
| .shooting-star { | ||
| position: absolute; | ||
| width: 4px; | ||
| height: 4px; | ||
| background: white; | ||
| border-radius: 50%; | ||
| top: -4px; | ||
| left: -4px; | ||
| animation: shootingStar 1.5s linear; | ||
| animation-iteration-count: 1; | ||
| } | ||
| `; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Moving Inline Styles to External CSS
The styles variable contains CSS defined as a template literal and is injected into a <style> tag within the component. While this approach works, it can make the code harder to maintain and can lead to duplication if multiple components need similar styles.
Consider moving these styles to an external CSS file, a CSS module, or using a CSS-in-JS solution like styled-components or Emotion. This will improve code organization, enable reuse, and make it easier to manage styles across the project.
| const seededRandom = (function () { | ||
| const seed = 12345; // You can change this seed to get a different, but consistent, pattern | ||
| let state = seed; | ||
| return function () { | ||
| state = (state * 1664525 + 1013904223) % 4294967296; | ||
| return state / 4294967296; | ||
| }; | ||
| })(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move Seeded Random Function Outside Component to Avoid Re-initialization
The seededRandom function is defined inside the NightSky component, which means it will be re-initialized on every render. This can lead to inconsistent behavior or unnecessary computations.
Consider moving the seededRandom function outside of the component to ensure that the random sequence remains consistent across renders:
- const NightSky = () => {- const seededRandom = (function () {+ // Simple pseudo-random number generator+ const seededRandom = (function () {
const seed = 12345; // You can change this seed to get a different, but consistent, pattern
let state = seed;
return function () {
state = (state * 1664525 + 1013904223) % 4294967296;
return state / 4294967296;
};
})();
+ const NightSky = () => {
// ... rest of the componentThis change ensures that the random number generator maintains its state between renders, leading to consistent star patterns.
📝 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.
| constseededRandom=(function(){ | |
| constseed=12345;// You can change this seed to get a different, but consistent, pattern | |
| letstate=seed; | |
| returnfunction(){ | |
| state=(state*1664525+1013904223)%4294967296; | |
| returnstate/4294967296; | |
| }; | |
| })(); | |
| // Simple pseudo-random number generator | |
| constseededRandom=(function(){ | |
| constseed=12345;// You can change this seed to get a different, but consistent, pattern | |
| letstate=seed; | |
| returnfunction(){ | |
| state=(state*1664525+1013904223)%4294967296; | |
| returnstate/4294967296; | |
| }; | |
| })(); | |
| constNightSky=()=>{ | |
| // ... rest of the component |
| const createShootingStar = () => { | ||
| const star = document.createElement("div"); | ||
| star.className = "shooting-star"; | ||
| document.querySelector(".night-sky-container")!.appendChild(star); |
There was a problem hiding this comment.
Ensure Safe Access to DOM Elements
At line 84, the code uses document.querySelector(".night-sky-container")! with a non-null assertion operator. If the element with the class .night-sky-container is not found, this will throw an error at runtime.
Consider adding a null check to ensure that the element exists before attempting to append the shooting star:
- document.querySelector(".night-sky-container")!.appendChild(star);+ const container = document.querySelector(".night-sky-container");+ if (container) {+ container.appendChild(star);+ }Alternatively, refactoring to avoid direct DOM manipulation (as suggested earlier) will also resolve this issue.
📝 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.
| document.querySelector(".night-sky-container")!.appendChild(star); | |
| constcontainer=document.querySelector(".night-sky-container"); | |
| if(container){ | |
| container.appendChild(star); | |
| } |
| const star = document.createElement("div"); | ||
| star.className = "shooting-star"; | ||
| document.querySelector(".night-sky-container")!.appendChild(star); | ||
| star.addEventListener("animationend", () => { | ||
| star.remove(); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Direct DOM Manipulation in React Components
The NightSky component directly manipulates the DOM using document.createElement and document.querySelector to create and append shooting stars. This approach bypasses React's virtual DOM and can lead to unexpected behavior or conflicts.
Consider refactoring this logic to utilize React's state and rendering mechanisms. By managing shooting stars through React state, you maintain the benefits of React's declarative paradigm and avoid potential issues with direct DOM manipulation.
Here's an example of how you might refactor the code:
const NightSky = () => {
+ const [shootingStars, setShootingStars] = useState<number[]>([]);
useEffect(() => {
const createShootingStar = () => {
- const star = document.createElement("div");- star.className = "shooting-star";- document.querySelector(".night-sky-container")!.appendChild(star);-- star.addEventListener("animationend", () => {- star.remove();- });+ const id = Date.now();+ setShootingStars((prev) => [...prev, id]);+ setTimeout(() => {+ setShootingStars((prev) => prev.filter((starId) => starId !== id));+ }, 1500); // Match the duration of the shooting star animation
};
const interval = setInterval(() => {
if (Math.random() < 0.3) {
// 30% chance every 3 seconds
createShootingStar();
}
}, 3000);
return () => clearInterval(interval);
}, []);
return (
<div className="night-sky-container relative h-full w-full">
<style>{styles}</style>
<svg
xmlns="http://www.w3.org/2000/svg"
viewBox="0 0 100 100"
preserveAspectRatio="xMidYMid slice"
style={{ background: "transparent", width: "100%", height: "100%" }}
className="absolute inset-0"
>
{generateStars()}
{generateAnimatedStars()}
</svg>
+ {shootingStars.map((id) => (+ <div key={id} className="shooting-star" />+ ))}
</div>
);
};This refactor keeps all DOM manipulations within React's virtual DOM, promoting better maintainability and consistency.
📝 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.
| conststar=document.createElement("div"); | |
| star.className="shooting-star"; | |
| document.querySelector(".night-sky-container")!.appendChild(star); | |
| star.addEventListener("animationend",()=>{ | |
| star.remove(); | |
| }); | |
| }; | |
| const[shootingStars,setShootingStars]=useState<number[]>([]); | |
| useEffect(()=>{ | |
| constcreateShootingStar=()=>{ | |
| constid=Date.now(); | |
| setShootingStars((prev)=>[...prev,id]); | |
| setTimeout(()=>{ | |
| setShootingStars((prev)=>prev.filter((starId)=>starId!==id)); | |
| },1500);// Match the duration of the shooting star animation | |
| }; | |
| constinterval=setInterval(()=>{ | |
| if(Math.random()<0.3){ | |
| // 30% chance every 3 seconds | |
| createShootingStar(); | |
| } | |
| },3000); | |
| return()=>clearInterval(interval); | |
| },[]); | |
| return( | |
| <divclassName="night-sky-container relative h-full w-full"> | |
| <style>{styles}</style> | |
| <svg | |
| xmlns="http://www.w3.org/2000/svg" | |
| viewBox="0 0 100 100" | |
| preserveAspectRatio="xMidYMid slice" | |
| style={{background: "transparent",width: "100%",height: "100%"}} | |
| className="absolute inset-0" | |
| > | |
| {generateStars()} | |
| {generateAnimatedStars()} | |
| </svg> | |
| {shootingStars.map((id)=>( | |
| <divkey={id}className="shooting-star"/> | |
| ))} | |
| </div> | |
| ); |
✨ Codu Pull Request 💻
The hero landing wasn't optimized for mobile or SEO.
Pull Request details
Any Breaking changes
Associated Screenshots
Screen.Recording.2024-10-14.at.18.50.11.mov