fix(Label): align celebration animation stroke radius with small size#3353
Open
yousefkadah wants to merge 4 commits into
Open
fix(Label): align celebration animation stroke radius with small size#3353yousefkadah wants to merge 4 commits into
yousefkadah wants to merge 4 commits into
Conversation
The celebration animation hardcoded a 4px corner radius, which overflowed the 2px-radius corners of small labels. Thread the actual border-radius into the SVG path generator so the animated stroke matches the label's rendered shape. Closes mondaycom#3128 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Code Review by Qodo
Context used 1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
18
to
23
| /** | ||
| * Border radius of the wrapped label, in pixels. Used to draw the animated stroke so it follows | ||
| * the label's actual rounded corners (e.g. small labels use a 2px radius, medium labels 4px). | ||
| */ | ||
| borderRadius?: number; | ||
| } |
Contributor
There was a problem hiding this comment.
1. labelcelebrationanimationprops defined inline 📘 Rule violation ⚙ Maintainability
borderRadius was added to an inline props interface inside LabelCelebrationAnimation.tsx instead of using a dedicated *.types.ts file extending VibeComponentProps. This breaks the required types-first component structure and increases long-term API inconsistency risk.
Agent Prompt
## Issue description
`LabelCelebrationAnimationProps` is defined/extended inline in `LabelCelebrationAnimation.tsx` (updated in this PR), but compliance requires component props to live in a dedicated `*.types.ts` file and extend `VibeComponentProps`.
## Issue Context
This PR added `borderRadius?: number` to `LabelCelebrationAnimationProps`, so this is a good time to align the component with the required types-first structure.
## Fix Focus Areas
- packages/core/src/components/Label/LabelCelebrationAnimation.tsx[18-23]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+25
to
+29
| function LabelCelebrationAnimation({ | ||
| children, | ||
| onAnimationEnd, | ||
| borderRadius = DEFAULT_BORDER_RADIUS | ||
| }: LabelCelebrationAnimationProps) { |
Contributor
There was a problem hiding this comment.
2. labelcelebrationanimation not forwardref 📘 Rule violation ⚙ Maintainability
LabelCelebrationAnimation is still declared as a plain function component rather than being implemented via React forwardRef. This violates the required component implementation pattern and makes ref usage inconsistent across the system.
Agent Prompt
## Issue description
`LabelCelebrationAnimation` is implemented as a plain function component, but compliance requires using `React.forwardRef` for components.
## Issue Context
This PR changed the component signature to add `borderRadius`, so the component is already being modified and can be refactored to the required `forwardRef` pattern.
## Fix Focus Areas
- packages/core/src/components/Label/LabelCelebrationAnimation.tsx[25-29]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Read the four corner radii from the child via getComputedStyle instead of hardcoding values in TS. This keeps the stroke aligned with the label's shape when CSS tokens are overridden, and correctly handles labels with a leg, where one corner is squared off. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Persistent review updated to latest commit ae9c031 |
childRef points to the outer wrapper span, but border-radius is applied to the nested Text element marked with data-celebration-text. Resolve that inner element via querySelector before reading computed radii so the SVG stroke matches the rendered shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Persistent review updated to latest commit d340035 |
Member
|
@yousefkadah thank you so much for this PR! Screen.Recording.2026-05-25.at.16.34.49.mov |
Author
|
Hey @talkor, thanks! 🙏 The jumpy top line came from two small things in the stroke path: the top edge wasn't lined up evenly with the corners, and the length I used for the dash animation was a few pixels longer than the actual path, so the line overshot at the top and jumped. I fixed both — now the edges line up and the dash length matches the real path, so the loop should be smooth. Could you take another look when you get a chance? |
Contributor
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
Inset every edge of the celebration path by half the stroke width consistently, and subtract that inset from the perimeter so stroke-dasharray equals the actual path length. Fixes the slightly-off top edge and the jumpy looping stroke.
yousefkadah
force-pushed
the
fix/label-celebration-animation-small-size
branch
from
a4b4ae6 to
79d06e6
Compare
Contributor
|
Code review by qodo was updated up to the latest commit 79d06e6 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Fixes the celebration animation overflowing on small labels — it was drawing 4px corners around a 2px-radius box. Now passes the label's actual border-radius through to the animation.
Closes #3128
PR Type
Bug fix
Description
Dynamically read corner radii from rendered element instead of hardcoding
Support non-uniform border-radius on labels with legs or CSS overrides
Fix celebration stroke path to match label's actual shape and size
Align stroke-dasharray with drawn path length to prevent animation jumps
Diagram Walkthrough
File Walkthrough
LabelCelebrationAnimation.tsx
Dynamic corner radii and path alignment fixespackages/core/src/components/Label/LabelCelebrationAnimation.tsx
DEFAULT_BORDER_RADIUSconstantCornerRadiiinterface to track individual corner radii (tl, tr,br, bl)
readCornerRadii()function to dynamically read computedborder-radius values from the styled inner element via
getComputedStyle()getPath()to accept individual corner radii and generate SVGpath with proper insets for stroke width, ensuring smooth animation
getPerimeter()to calculate perimeter based on actual cornerradii and stroke width, fixing animation loop alignment
[data-celebration-text]element for accurate border-radius values