Social links : Default background color problem fixed. Fix #34677

[HILAYTRIVEDI]

I tested the code with different themes which are default provided by WordPress. No other functionalities are affected by the changes. Two new useEffect() is added to the packages\block-library\src\social-links/edit.js for the default and pill classes. As in the pill also the same issue arises.

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 of preveIconBackgroundColorValue for the previously set value of the background.

Bug fix #34677

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[github-actions]

👋 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.

[stokesman]

Hi @HILAYTRIVEDI, thanks taking this on. It looks like this resolves the issue. I think it'd better done without adding another block attribute because those are meant for things that need to persist across saves. Besides that, I think we can make this a bit simpler. I've left a couple of comments to elaborate but let me know if anything isn't clear.

[stokesman]

Thanks for the updates @HILAYTRIVEDI 👍 It's much nicer with the reduced changes. I've left some more feedback.

It'd be great to have another reviewer have a look but I see some others have been requested so I'll give them some time to get to it before pinging them again.

[stokesman]

This works well in my testing and I've left one more suggestion but it seems optional. Perhaps another reviewer can weigh in on that. Either way, I think we could merge this. Thanks @HILAYTRIVEDI!

[stokesman]

So this doesn't seem necessary (as mentioned before: #34834 (comment)) but it may be better to have all the consumed variables in the hook dependencies and it doesn't end up adding much complexity.

Suggested change

	backgroundBackup.current = {
	iconBackgroundColor,
	iconBackgroundColorValue,
	customIconBackgroundColor,
	};
	setAttributes( {
	iconBackgroundColor: undefined,
	customIconBackgroundColor: undefined,
	iconBackgroundColorValue: undefined,
	} );
	} else {
	setAttributes( { ...backgroundBackup.current } );
	}
	}, [ logosOnly, setAttributes ] );
	setAttributes( {
	iconBackgroundColor: undefined,
	customIconBackgroundColor: undefined,
	iconBackgroundColorValue: undefined,
	} );
	// Restore the attributes from backup on cleanup.
	return () => setAttributes( { ...backgroundBackup.current } );
	}
	// Backup icon background attributes.
	backgroundBackup.current = {
	iconBackgroundColor,
	iconBackgroundColorValue,
	customIconBackgroundColor,
	};
	}, [
	logosOnly,
	setAttributes,
	iconBackgroundColor,
	iconBackgroundColorValue,
	customIconBackgroundColor,
	] );

[HILAYTRIVEDI]

Agreed.

[HILAYTRIVEDI]

@stokesman I am happy that I get chance for contributing.

[Mamaduka]

Hi, @HILAYTRIVEDI

Can you rebase this branch on top of the trunk? I think we can merge this once all checks are green.

[HILAYTRIVEDI]

Hi @Mamaduka , I am unable to rebase the branch.