Website: Implement glowing cursor effect #1009
Conversation
IPogorelova
requested review from
andrashee,
mkue,
ssandino and
socialincome-dev
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe pull request introduces a glow hover effect system for UI components. It adds a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Component
participant GlowHoverHook
participant GlowHoverEffect
User->>Component: Hover/Interact
Component->>GlowHoverHook: Apply Hover Effect
GlowHoverHook->>GlowHoverEffect: Initialize Effect
GlowHoverEffect-->>Component: Update Visual Styling
GlowHoverEffect->>GlowHoverEffect: Animate Glow
Possibly related issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (2)
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: 3
🧹 Nitpick comments (3)
ui/src/components/use-glow-hover/glow-hover-effect.ts (1)
124-124: Avoid assignments within arrow function expressionsUsing assignments within arrow function expressions without braces can be confusing and reduce code readability.
Apply the following diff to improve clarity:
-onIdUpdate: (newId) => (lightSizeEnterAnimationId = newId), +onIdUpdate: (newId) => { + lightSizeEnterAnimationId = newId; +},Repeat this change for lines 158, 177, and 196.
Also applies to: 158-158, 177-177, 196-196
🧰 Tools
🪛 Biome (1.9.4)
[error] 124-124: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
ui/src/components/use-glow-hover/use-glow-hover.ts (1)
13-13: OptimizeuseEffectdependencies to prevent unnecessary re-rendersUsing
Object.values(options)in the dependency array may cause the effect to run on every render ifoptionsis not memoized, leading to performance issues.Consider memoizing the
optionsobject or explicitly listing each option in the dependency array.ui/src/components/use-glow-hover/linear-animation.ts (1)
14-15: ClampinitialProgressto the range [0, 1]Currently,
initialProgresscan be outside the range[0, 1], which may cause unexpected behavior in the animation.Apply this diff to ensure
initialProgressis within the valid range:}: LinearAnimationParams) => { + initialProgress = Math.min(Math.max(initialProgress, 0), 1); if (time === 0) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ui/src/components/containers/base-container.tsx(1 hunks)ui/src/components/use-glow-hover/glow-hover-effect.ts(1 hunks)ui/src/components/use-glow-hover/index.ts(1 hunks)ui/src/components/use-glow-hover/linear-animation.ts(1 hunks)ui/src/components/use-glow-hover/use-glow-hover.ts(1 hunks)ui/src/index.ts(1 hunks)website/src/app/[lang]/[region]/(website)/(home)/(sections)/overview.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- ui/src/components/use-glow-hover/index.ts
🧰 Additional context used
🪛 GitHub Actions: Website
ui/src/components/containers/base-container.tsx
[error] 14-14: Property 'wrapperClassName' does not exist on type '{ backgroundColor?: BackgroundColor | undefined; wrapperRef?: Ref | undefined; } & HTMLAttributes'
🪛 Biome (1.9.4)
ui/src/components/use-glow-hover/glow-hover-effect.ts
[error] 124-124: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 158-158: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 177-177: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 196-196: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (2)
ui/src/index.ts (1)
30-30: LGTM!The new export follows the existing pattern and maintains alphabetical order.
website/src/app/[lang]/[region]/(website)/(home)/(sections)/overview.tsx (1)
14-26: Remove temporary example code.The TODO comment indicates this code should be removed before merge. Please remove this example usage of GLowHoverContainer.
| export const GLowHoverContainer = React.forwardRef<HTMLDivElement, Omit<BaseContainerProps, 'wrapperRef'>>( | ||
| ({ className, ...props }, ref) => { | ||
| const refCard = useGlowHover({ lightColor: '#CEFF00' }); | ||
|
|
||
| return ( | ||
| <BaseContainer | ||
| wrapperClassName={twMerge('theme-blue', className)} | ||
| ref={ref} | ||
| {...props} | ||
| wrapperRef={refCard as React.Ref<HTMLDivElement>} | ||
| /> | ||
| ); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and configuration of GLowHoverContainer.
Two suggestions for improvement:
- Avoid type casting of refCard
- Make the light color configurable
-export const GLowHoverContainer = React.forwardRef<HTMLDivElement, Omit<BaseContainerProps, 'wrapperRef'>>(
+type GlowHoverContainerProps = Omit<BaseContainerProps, 'wrapperRef'> & {
+ lightColor?: string;
+};
+
+export const GLowHoverContainer = React.forwardRef<HTMLDivElement, GlowHoverContainerProps>(
- ({ className, ...props }, ref) => {
+ ({ className, lightColor = '#CEFF00', ...props }, ref) => {
- const refCard = useGlowHover({ lightColor: '#CEFF00' });
+ const refCard = useGlowHover({ lightColor });
return (
<BaseContainer
wrapperClassName={twMerge('theme-blue', className)}
ref={ref}
{...props}
- wrapperRef={refCard as React.Ref<HTMLDivElement>}
+ wrapperRef={refCard}
/>
);
},
);📝 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.
| export const GLowHoverContainer = React.forwardRef<HTMLDivElement, Omit<BaseContainerProps, 'wrapperRef'>>( | |
| ({ className, ...props }, ref) => { | |
| const refCard = useGlowHover({ lightColor: '#CEFF00' }); | |
| return ( | |
| <BaseContainer | |
| wrapperClassName={twMerge('theme-blue', className)} | |
| ref={ref} | |
| {...props} | |
| wrapperRef={refCard as React.Ref<HTMLDivElement>} | |
| /> | |
| ); | |
| }, | |
| type GlowHoverContainerProps = Omit<BaseContainerProps, 'wrapperRef'> & { | |
| lightColor?: string; | |
| }; | |
| export const GLowHoverContainer = React.forwardRef<HTMLDivElement, GlowHoverContainerProps>( | |
| ({ className, lightColor = '#CEFF00', ...props }, ref) => { | |
| const refCard = useGlowHover({ lightColor }); | |
| return ( | |
| <BaseContainer | |
| wrapperClassName={twMerge('theme-blue', className)} | |
| ref={ref} | |
| {...props} | |
| wrapperRef={refCard} | |
| /> | |
| ); | |
| }, |
There was a problem hiding this comment.
@ssandino hi!
Could you please tell if you need the glowing color to be configurable?
As I can see now, the blue color mostly used throughout the site to make banners and accent sections and yellow cursor glow fits perfectly here. However, there's no problem adding prop for a custom color.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| <GLowHoverContainer className="mb-16 mt-8 flex flex-col items-center justify-center md:mb-48 md:mt-16"> | ||
| <Typography size="3xl" weight="medium" className="my-10 text-center"> | ||
| {translator.t('section-2.title-1')} | ||
| </Typography> | ||
| <div className="mb-8 text-center"> | ||
| {translator.t<{ text: string; color?: FontColor }[]>('section-2.title-2').map((title, index) => ( | ||
| <Typography as="span" size="3xl" weight="medium" color={title.color} key={index}> | ||
| {title.text}{' '} | ||
| </Typography> | ||
| ))} | ||
| </div> | ||
| </GLowHoverContainer> | ||
|
|
||
| <BaseContainer className="mx-auto mb-16 mt-8 flex w-4/5 flex-col items-center justify-center md:mb-48 md:mt-16 lg:w-3/5"> | ||
| <Typography size="3xl" weight="medium" className="my-10 text-center"> | ||
| {translator.t('section-2.title-1')} | ||
| </Typography> | ||
| <div className="mb-8 text-center"> | ||
| {translator.t<{ text: string; color?: FontColor }[]>('section-2.title-2').map((title, index) => ( | ||
| <Typography as="span" size="3xl" weight="medium" color={title.color} key={index}> | ||
| {title.text}{' '} | ||
| </Typography> | ||
| ))} | ||
| </div> | ||
| <Typography size="3xl" weight="medium" className="mb-5 mt-12 text-center"> | ||
| {translator.t('section-2.title-3')} | ||
| </Typography> | ||
| <ol className="mb-8 list-decimal"> | ||
| <li> | ||
| <Typography as="span">{translator.t('section-2.text-3.1')}</Typography> | ||
| </li> | ||
| <li> | ||
| <Typography as="span">{translator.t('section-2.text-3.2')}</Typography> | ||
| </li> | ||
| </ol> | ||
| <Typography size="3xl" weight="medium" className="mb-5 mt-12 text-center"> | ||
| {translator.t('section-2.title-4')} | ||
| </Typography> | ||
| <Typography className="mb-8">{translator.t('section-2.text-4')}</Typography> | ||
| <Typography size="3xl" weight="medium" className="mb-5 mt-12 text-center"> | ||
| {translator.t('section-2.title-5')} | ||
| </Typography> | ||
| <Typography className="w-full">{translator.t('section-2.text-5')}</Typography> | ||
| </BaseContainer> |
There was a problem hiding this comment.
Avoid content duplication.
The same content is duplicated between GLowHoverContainer and BaseContainer. Choose one container based on the intended design.
There was a problem hiding this comment.
JFI: this part of duplicated content will be removed before merge
There was a problem hiding this comment.
@IPogorelova Thanks for clarifying! I see the TODO comment now, and understand this is just a temporary example.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
Locally the effect works very nicely: |
|
Visit the preview URL for this PR (updated for commit b3b8e29): https://si-admin-staging--pr1009-implement-glowing-cu-0qtybgf8.web.app (expires Tue, 21 Jan 2025 18:13:21 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b7b0969384059dce6ea8fad1ee1d1737e54e6676 |
|
The deployment on Vercel preview throws following error:
And the co-pilot bug report says: |
… into implement-glowing-cursor-effect
This PR introduces a new component,
GlowHoverContainer, which extendsBaseContainerwith a glowing cursor effect.Issue: #987
Changes:
GlowHoverContainer: a new component that wrapsBaseContainerand adds the glowing cursor effect.BaseContainer: minor adjustments to enable the glowing cursor functionality:backgroundColorproperty, which was previously used on both the parent and child<div>s within the component. This change appears to have no impact on existing functionality, as I've found only 1 usage ofbackgroundColorproperty in the project (on the /arts page) and it works fine after component's change.GlowHoverContainerusage on the main page below the Hero section. (TODO: Remove this example before merging)Note:
npm run format-codeproduced unexpected changes across all repo files, primarily related to line breaks. To avoid unintended consequences, I did not commit these changes.I understand that consistent code formatting is important. However, I'm encountering difficulties with the current formatting command locally. If you have any suggestions or have ever encountered the same issue, I'd really appreciate any advice.
Hope this helps!
Summary by CodeRabbit
New Features
GLowHoverContainercomponent for enhanced visual interaction.Improvements
Technical Enhancements