-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Social Icons: Avoid loss of previously selected background color when switching back from "Logos Only" style #39276
Social Icons: Avoid loss of previously selected background color when switching back from "Logos Only" style #39276
Conversation
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @HILAYTRIVEDI! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
Hi @HILAYTRIVEDI, this looks to be the same branch as #34834. It's still a great many commits behind trunk. I can checkout the branch and see about rebasing it Update: I see they are different branches and the other didn't rebase without conflicts while this one did. So fine to have this one open I guess. |
0d5d1c4
to
bd60594
Compare
Thank you @stokesman , I closed the previous pull request. |
a2dcc0d
to
9312822
Compare
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've tested and it works as intended. LGTM! Disclaimer: I fixed one thing and made a couple of quality/style changes besides.
My confidence in this is high so unless anyone chimes in with a reason not to I think I'll merge it soon. CC @Mamaduka. |
The changes look good to me. Thank you, @HILAYTRIVEDI and @stokesman. |
I tested the code with different themes which are default provided by WordPress. No other functionalities are affected by the changes.
After the changes, I tested it with the different combinations of the use of the styling pattern, and it's working fine.
Video Demo Of Fix
https://www.awesomescreenshot.com/video/5232616?key=888676e6a53d9db85397b4430d64c810
Types of changes
The use effect in the file uses the previous state of the
iconBackgroundColorValue
and while the class changes it uses the value ofpreveIconBackgroundColorValue
for the previously set value of the background.Bugfix #34677
Checklist: