fix: button reveal animation (DSN-2529) #315
Closed
Conversation
This file contains 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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Your org requires the Graphite merge queue for merging into masterAdd the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
116
to
140
| <div | ||
| className={buttonAnimation} | ||
| style={{ | ||
| animationDelay: `${index * 0.1}s`, | ||
| }} | ||
| > | ||
| <Button variant={ButtonVariant.INLINE} onClick={() => runtime?.interact(request, name)} key={index}> | ||
| {name} | ||
| </Button> | ||
| </div> |
There was a problem hiding this comment.
The React key prop needs to be moved from the Button to the wrapping div element to ensure proper list reconciliation. Currently:
<div className={buttonAnimation}>
<Button key={index} ...>Should be:
<div key={index} className={buttonAnimation}>
<Button ...>This change will help React correctly track and update the animated button elements in the list.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
gillyb
approved these changes
Nov 15, 2024
mikaalnaik
force-pushed
the
mikaal/button-animation/DSN-2529
branch
from
24e5806
to
9a243d6
Compare
|
Merge activity
|
mikaalnaik
added a commit
that referenced
this pull request
Nov 15, 2024
<!-- You can erase any parts of this template not applicable to your Pull Request. --> **Fixes or implements VF-XXX** ### Brief description. What is this change? <!-- Build up some context for your teammates on the changes made here and potential tradeoffs made and/or highlight any topics for discussion --> ### Implementation details. How do you make this change? <!-- Explain the way/approach you follow to make this change more deeply in order to help your teammates to understand much easier this change --> ### Setup information <!-- Notes regarding local environment. These should note any new configurations, new environment variables, etc. --> ### Deployment Notes <!-- Notes regarding deployment the contained body of work. These should note any db migrations, etc. --> ### Related PRs <!-- List related PRs against other branches --> - https://github.com/voiceflow/XXXXXXXXX/pull/123 ### Checklist - [ ] Breaking changes have been communicated, including: - New required environment variables - Renaming of interfaces (API routes, request/response interface, etc) - [ ] New environment variables have [been deployed](https://www.notion.so/voiceflow/Add-Environment-Variables-be1b0136479f45f1adece7995a7adbfb) - [ ] Appropriate tests have been written - Bug fixes are accompanied by an updated or new test - New features are accompanied by a new test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes or implements VF-XXX
Brief description. What is this change?
Implementation details. How do you make this change?
Setup information
Deployment Notes
Related PRs
Checklist