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.
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
fix(CodeSnippet): use tooltip styles for feedback and add animation #4741
fix(CodeSnippet): use tooltip styles for feedback and add animation #4741
Changes from all commits
3668ca9
b5bf1dc
2af823a
56eef79
e791e70
2acd1a2
790a2c4
212c75a
cb3e742
78ce55b
694e9da
32d0f2f
4b2fddb
c8ed9f2
02b8729
016453b
e8c4cf7
9dd0f2e
273a241
8be5b17
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Would it make sense for this work to be in an effect parameterized by
animation
changing? Something like: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.
does this eliminate our debouncing then? I moved towards using debounce to eliminate the need for managing timers and refs
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.
Got it, is
handleFadeOut
called multiple time which is why we need debounce here?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.
yeah the tooltip needs to persist if the button is clicked again and should fade out after the timeout expires for the most recent click of the button. with
useEffect
all clicks following the first click are ignored until the tooltip fades out (based on the timeout from the very first click). so we debounce the click and fadeout handlers because they can fire multiple timesThere 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.
Hmm, this seems like a use-case we could achieve with
useEffect
, right? What would make the clicks become ignored? It seemed like on click we want to update the animation state and then enqueue a timeout, cancelling it if the button is clicked and creating a new one. Is there something else that I'm missing here? 👀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.
if the timeout management occurs in
useEffect
doesn't that require another render? patching withuseEffect
results inclearTimeout
firing after the timeout has already completedThere 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.
Style-nit: use function declarations for event handlers
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.
just wondering is there a reason behind this? a function declaration would be hoisted, so in that case we might as well be advocating for all function declarations to be made at the top of their scopes
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.
I think at this point it's just as a style consistency point tbh, if we have hard numbers one way or another could definitely update the style guide!
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.
if it isn't part of the style guide yet I think there should be a discussion about that rule. I lean more towards function expressions in these cases to avoid function hoisting
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.
@emyarod it is in the style guide over in: https://github.com/carbon-design-system/carbon/blob/master/docs/style.md#writing-a-component
Feel free to bring up reasons against it in our Slack channel or a PR! We only use this style since it's what React is currently using in their official documentation and there weren't any glaring reasons to stray from it.
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.
I don't see anything about avoiding function expressions for event handlers in the style guide or the React documentation. can you elaborate on that?