fix(CodeSnippet): use tooltip styles for feedback and add animation #4741
Conversation
ghost
requested review from
|
Deploy preview for the-carbon-components ready! Built with commit 8be5b17 https://deploy-preview-4741--the-carbon-components.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit 8be5b17 |
|
Deploy preview for carbon-components-react ready! Built with commit 8be5b17 https://deploy-preview-4741--carbon-components-react.netlify.com |
emyarod
force-pushed
the
4604-code-snippet-inherit-tooltip
branch
from
086aee1
to
8e388de
Compare
| }, | ||
| [onClick, handleFadeOut] | ||
| ); | ||
| const handleAnimationEnd = event => { |
There was a problem hiding this comment.
Style-nit: use function declarations for event handlers
There was a problem hiding this comment.
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.
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.
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.
@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.
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?
| [`${prefix}--copy-btn--animating`]: animation, | ||
| [`${prefix}--copy-btn--${animation}`]: animation, | ||
| }); | ||
| const handleFadeOut = useCallback( |
There was a problem hiding this comment.
Would it make sense for this work to be in an effect parameterized by animation changing? Something like:
useEffect(() => {
if (animation !== 'fade-in') {
return;
}
const timeoutId = window.setTimeout(() => {
setAnimation('fade-out');
}, [feedbackTimeout]);
return () => {
window.clearTimeout(timeoutId);
};
}, [animation, feedbackTimeout]);There was a problem hiding this comment.
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.
Got it, is handleFadeOut called multiple time which is why we need debounce here?
There was a problem hiding this comment.
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 times
There was a problem hiding this comment.
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.
if the timeout management occurs in useEffect doesn't that require another render? patching with useEffect results in clearTimeout firing after the timeout has already completed
emyarod
force-pushed
the
4604-code-snippet-inherit-tooltip
branch
from
69298b8
to
fb6c6fa
Compare
|
We've marked this issue as stale because there hasn't been any activity for a couple of weeks. If there's no further activity on this issue in the next three days then we'll close it. You can keep the conversation going with just a short comment. Thanks for your contributions. |
stale
bot
added
the
status: inactive
asudoh
removed
the
status: inactive
emyarod
force-pushed
the
4604-code-snippet-inherit-tooltip
branch
2 times, most recently
from
8692aa9
to
c449668
Compare
emyarod
force-pushed
the
4604-code-snippet-inherit-tooltip
branch
from
9d6d9da
to
a84ee82
Compare
emyarod
force-pushed
the
4604-code-snippet-inherit-tooltip
branch
from
a84ee82
to
273a241
Compare
asudoh
dismissed
joshblack’s stale review
Resetting the review status as all review comments seem to have been addressed.
|
@asudoh Please confirm if review comments have been addressed before dismissing a review. The most recent comments were from 2 days ago when the previous interaction was a month back and I had not had a chance to respond yet. Feel free to reach out on Slack in our squad channel 👍 |
|
I expect responses to review comments are treated as request for re-reviews, and expect that reviewers respond in timely manner. Otherwise reviews are seen as stale. |
|
@joshblack @asudoh i think clicking the “request re-review” button should count as requesting a re-review. We all get a lot of notifications, missing a single reply shouldn’t count as an implicit approval. |
|
I read through all notifications of this repo. |
|
Just wanted to quickly jump in here and say that yes we should all be timely in reviewing and responding to PRs/comments, but we should also feel empowered to reach out on slack and poke someone if it seems they’ve missed something. (especially around the holidays when notifications could easily get missed) 🙏 |
Closes #4604
Closes #4954
ref #4711 #4715
This PR styles the code snippet and
<Copy />component to use tooltip visual style rules along with the tooltip fade animationsTesting / Reviewing
Ensure each code snippet variant looks and functions as expected