Add Sign in with Google Settings View screen. #9685
Conversation
|
Build files for 51ac04b are ready:
|
There was a problem hiding this comment.
Thanks @tofumatt – perhaps I'm missing an important detail but it seems like the infra related to the site registration setting could be simplified. Otherwise left a few other points throughout.
| const dismissedItems = select( CORE_USER ).getDismissedItems(); | ||
|
|
||
| return dismissedItems?.some( | ||
| ( item ) => | ||
| item === 'sign-in-with-google-anyone-can-register-notice' | ||
| ); |
There was a problem hiding this comment.
Not sure about the formatting, but this should be something we can simplify with the dedicated selector
| const dismissedItems = select( CORE_USER ).getDismissedItems(); | |
| return dismissedItems?.some( | |
| ( item ) => | |
| item === 'sign-in-with-google-anyone-can-register-notice' | |
| ); | |
| return select( CORE_USER ).isItemDismissed( | |
| 'sign-in-with-google-anyone-can-register-notice' | |
| ); |
|
|
||
| // If Sign in with Google does not have a client ID, do not display the | ||
| // settings view. | ||
| if ( ! clientID?.length ) { |
There was a problem hiding this comment.
Why the length check on a string? It works of course but I don't think there is a risk of it being misinterpreted. I think we can simply if ( ! clientID )?
| { !! buttonTextLabel && ( | ||
| <div className="googlesitekit-settings-module__meta-item"> | ||
| <h5 className="googlesitekit-settings-module__meta-item-type"> | ||
| { __( 'Button text', 'google-site-kit' ) } | ||
| </h5> | ||
| <p className="googlesitekit-settings-module__meta-item-data"> | ||
| <DisplaySetting value={ buttonTextLabel } /> | ||
| </p> | ||
| </div> | ||
| ) } | ||
|
|
||
| { !! buttonThemeLabel && ( | ||
| <div className="googlesitekit-settings-module__meta-item"> | ||
| <h5 className="googlesitekit-settings-module__meta-item-type"> | ||
| { __( 'Button theme', 'google-site-kit' ) } | ||
| </h5> | ||
| <p className="googlesitekit-settings-module__meta-item-data"> | ||
| <DisplaySetting value={ buttonThemeLabel } /> | ||
| </p> | ||
| </div> | ||
| ) } | ||
|
|
||
| { !! buttonShapeLabel && ( |
There was a problem hiding this comment.
Is it possible the values would ever be empty? We already have the early return at the top if there is no clientID so this seems unlikely. Given they all have defaults I don't think it is (realistically at least). Also DisplaySetting is intended to account for the case where the value is not loaded yet by holding the vertical space.
| { anyoneCanRegisterNoticeDismissed !== undefined && | ||
| anyoneCanRegisterNoticeDismissed !== true && |
There was a problem hiding this comment.
Maybe just the one condition?
| { anyoneCanRegisterNoticeDismissed !== undefined && | |
| anyoneCanRegisterNoticeDismissed !== true && | |
| { anyoneCanRegisterNoticeDismissed === false && |
| export const Default = Template.bind( null ); | ||
| Default.storyName = 'Default'; | ||
| Default.scenario = { | ||
| label: 'Modules/SignInWithGoogle/Settings/SettingsView/Default', |
There was a problem hiding this comment.
We almost never need to set the label manually, this was more for matching old existing scenario outputs during the migration. Its generated automatically based on the storyName and the default.title below.
| .dispatch( MODULES_SIGN_IN_WITH_GOOGLE ) | ||
| .receiveGetSettings( { | ||
| clientID: | ||
| 'example-client-id-123123123.apps.usercontent.com', |
There was a problem hiding this comment.
| 'example-client-id-123123123.apps.usercontent.com', | |
| 'example-client-id-123123123.apps.googleusercontent.com', |
| */ | ||
| protected function get_datapoint_definitions() { | ||
| return array( | ||
| 'GET:anyone-can-register' => array( 'service' => 'sign-in-with-google' ), |
There was a problem hiding this comment.
I see this is part of the IB but it seems like overkill for surfacing a site-wide setting. Why aren't we adding this to base data and surfacing it through core/site?
| <SettingsNotice | ||
| type={ TYPE_WARNING } |
There was a problem hiding this comment.
Not sure what happened here, but this looks quite different from the design
| @@ -0,0 +1,127 @@ | |||
| /** | |||
| * `modules/sign-in-with-google` data store: get "Anyone can register" value from WordPress. | |||
There was a problem hiding this comment.
As mentioned before, all of the infra around this value can likely be simplified into an extension of base data exposed via core/site.
|
One more detail I noticed, the badges have different padding and it's the "Beta" that doesn't match the design. 
Beta has It seems the reduced padding was applied to the new badge specifically – which seems incorrect actually – as the sizing should be more contextual rather than badge-specific. |
Summary
Addresses issue:
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist