-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
ToggleGroupControl
: remove animated backdrop
#38008
ToggleGroupControl
: remove animated backdrop
#38008
Conversation
ToggleGroupControl
: remove animated backdrop
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.
Thank you @KevinBatdorf for opening this PR!
The code changes test well, and overall LGTM. Would you be able to add an entry to the CHANGELOG?
The e2e failures don't seem related, maybe rebasing could be enough to have them pass.
@@ -49,7 +48,7 @@ function ToggleGroupControl( | |||
} = useContextSystem( props, 'ToggleGroupControl' ); | |||
const cx = useCx(); | |||
const containerRef = useRef(); | |||
const [ resizeListener, sizes ] = useResizeAware(); | |||
const [ resizeListener ] = useResizeAware(); |
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.
It looks like react-resize-aware
was only needed to resize the backdrop, and so I think we can remove this line and the import at the top of this file (cc'ing @ntsekouras for a confirmation!)
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.
That makes sense actually. The listener isn't very useful if the sizes
aren't being used. I removed it for now
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, it seems it can be safely removed.
Hey @ciampo I added the changelog and removed the listener. I also had to update the test to remove the listener too. Some of the tests are still failing, but I don't think they are related to this change? |
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.
Thanks for your work and all the iterations @KevinBatdorf! 👏
LGTM and I agree with @ciampo about this being a better short-term solution for the issue.
With a green CI let's 🚢
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.
Thank you Kevin, LGTM 🚀
Is there anything we can do to follow up and get the animated backdrop back in the next few days? Although I appreciate the need to fix separate bugs, this is a significant design regression. |
Implementing this transition effectively and in a bug-free manner can prove quite complicated (see the complexity of the code changes in the last bug fix attempt in #37507), and I can't guarantee that I'll be able to come up with a solution in just a few days. If, for the time being, you'd rather keep the older animation and reintroduce the bug that this PR fixed, I can go ahead an revert this PR. I will then work separately on a new implementation for the animation, and merge it when it's ready. |
We'll want to fix the bug, of course, but I'd prefer keeping the bug, and the backdrop until we can get both working at the same time. Thanks all for the work! |
Description
This PR removes the backdrop animation component and replaces it with a simple background color change
This PR is an alternative solution to #37507
Fixes #37506
How has this been tested?
Tested by adding a component with a background color and swapping between gradient and solid. Also, when the container is focused, pressing the left and right arrow keys properly applies the background color.
Screenshots
Screen.Recording.2022-01-15.at.5.01.13.PM.mov
Types of changes
Bug fix
Checklist:
*.native.js
files for terms that need renaming or removal).