Fix Carousel Transition Bug #5678
Merged
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
kevingill55
commented
Oct 11, 2021
kevingill55
commented
Oct 11, 2021
halocline
reviewed
Oct 12, 2021
There was a problem hiding this comment.
Looking great. Thank you, @kevingill55.
Just a couple minor comments.
Comment on lines
60
to
72
| CarouselChild.propTypes = { | ||
| fill: PropTypes.bool, | ||
| play: PropTypes.number, | ||
| index: PropTypes.number.isRequired, | ||
| activeIndex: PropTypes.number.isRequired, | ||
| priorActiveIndex: PropTypes.number, | ||
| }; | ||
|
|
||
| CarouselChild.defaultProps = { | ||
| fill: false, | ||
| play: undefined, | ||
| priorActiveIndex: undefined, | ||
| }; |
There was a problem hiding this comment.
Thoughts on keeping all the propType definitions consolidated in the Carousel/prop-types.js file for consistency? Can export CarouselChildPropType from there and consume it here. I think that would mimic the pattern implemented throughout the rest of the project.
Comment on lines
+22
to
+25
| /** | ||
| * This check will only be false onMount of the component. It ensures | ||
| * the initial active slide of the Carousel renders with no animation. | ||
| */ |
Comment on lines
39
to
42
| setTimeout( | ||
| () => setVisibility('hidden'), | ||
| theme.carousel.animation.duration, | ||
| ); |
There was a problem hiding this comment.
I think we need to clean up this useEffect's timer after the component unmounts.
Here are references to a couple Grommet instances where this pattern is used:
Other references:
There was a problem hiding this comment.
Ah yes! Great point
halocline
approved these changes
Oct 13, 2021
ericsoderberghp
approved these changes
Oct 13, 2021
|
Thanks for your contribution! |
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.
What does this PR do?
This fixes a regression introduced to the
Carouselcomponent that caused un-smooth and jarring transitions between Carousel slides. The issue is caused when the user changes the current slide, and thevisibilityproperty is set tohiddenbefore the slide can complete its animation of fading out. Thus, the current child is immediately removed while the next child is slowly sliding in.Where should the reviewer start?
The only changes were made in the
Carousel/Carousel.jsfile.What testing has been done on this PR?
The accessibility issue that this regression was introduced in is still fixed. All other functionality of the component is maintained.
How should this be manually tested?
Run storybook and see the Carousel stories. Make sure the accessibility issue that this PR fixed is still working. Also, ensure that the Carousel transitions between slides are smooth and as expected. Compare the transitions with what is on
master.What are the relevant issues?
#5675
Do the grommet docs need to be updated?
No
Should this PR be mentioned in the release notes?
Yes
Is this change backwards compatible or is it a breaking change?
Backwards compatible 👍