Skip to content
This repository was archived by the owner on Sep 21, 2023. It is now read-only.

Conversation

@sammce
Copy link

@sammce sammce commented Sep 25, 2021

I was tinkering and discovered that the styles are intended to not be altered.

The change uses Array.forEach() and Object.keys(), both of which are IE 9 compatible.

The test suites pass with the amended snapshot set to reflect the new user defined style prop.

Issue link

Edit:

After forking the repo and using the new inline style capabilities, I came to the conclusion that it might also be useful to be able to customise the style states, to keep a consistent design for all button states.

The new commit allows the following:

Screenshot 2021-09-27 at 19 23 35

Before

Screenshot 2021-09-25 at 17 33 41

After

Screenshot 2021-09-27 at 19 06 49

The implementation of the new style merging is to use a function, named 'addKeysTo' which takes 2 objects, copies a reference to the target object, and assigns each style from the user defined style object too it. The reason that I am using a function for this operation is to follow the DRY principle, and to minimize the bundle size.
This is to keep in line with the general layout of the project, instead of defining the function in google-login.js
@sammce
Copy link
Author

sammce commented Sep 27, 2021

I just tested using the rest operator instead of Object.keys for simplicity's sake, but the bundle size when doing so is 1.2KB larger than doing it the old fashioned way. I imagine this is due to the inclusion of the babel plugin @babel/plugin-proposal-object-rest-spread in the bundle. Just in case you were wondering why I didn't do it that way.

@sammce sammce marked this pull request as draft September 28, 2021 21:47
@sammce sammce closed this Sep 28, 2021
@sammce sammce reopened this Sep 28, 2021
@sammce sammce marked this pull request as ready for review September 28, 2021 22:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants