Implement settings edit screen #9047
Conversation
…nent' into enhancement/8841-settings-screen.
ankitrox
changed the title
|
Build files for 6d3de25 have been deleted. |
|
Size Change: +490 B (+0.03%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
| decorators: [ | ||
| ( Story, { args } ) => { | ||
| const setupRegistry = ( registry ) => { | ||
| enabledFeatures.add( 'rrmModule' ); |
There was a problem hiding this comment.
You should be able to use the WithRegistrySetup.features prop instead of adding the feature here via enabledFeatures.
| enabledFeatures.add( 'rrmModule' ); |
There was a problem hiding this comment.
Sorry @techanvil , but this does not seem to work as intended because WithRegistrySetup has signature
WithRegistrySetup( { func, children } ) {
...
}which does not seem to receive and do anything with features prop.
There was a problem hiding this comment.
Oh, yes, you're right. Sorry about that. We actually have this in a few places, which are obviously incorrect and we should fix. E.g.
site-kit-wp/assets/js/modules/ads/components/settings/SettingsEdit.stories.js
Lines 114 to 117 in e112780
Looking into it, the reason those stories are still working as expected is because the features are set in the parameters for the story, for example:
Providing features in parameters will automatically enable the features due to this decorator which is applied to all stories:
site-kit-wp/.storybook/preview.js
Lines 67 to 82 in e112780
What is means is that rather that my suggested fix you can instead apply the following to provide the features to all stories in this file:
export default {
title: 'Modules/ReaderRevenueManager/Components/Settings/SettingsEdit',
parameters: {
features: [ 'rrmModule' ],
},Could you please apply the above instead of my suggestions? One thing we don't tend to do is reference enabledFeatures directly in stories, because we have various abstractions around it.
I've created a story to remove up the existing attempted use of WithRegistrySetup.features: #9115
Cc @10upsimon
| }; | ||
|
|
||
| return ( | ||
| <WithRegistrySetup func={ setupRegistry }> |
There was a problem hiding this comment.
As above:
| <WithRegistrySetup func={ setupRegistry }> | |
| <WithRegistrySetup func={ setupRegistry } features={ [ 'rrmModule' ] }> |
|
|
||
| export default { | ||
| title: 'Modules/ReaderRevenueManager/Settings/SettingsEdit', | ||
| component: SettingsEdit, | ||
| title: 'Modules/ReaderRevenueManager/Components/Settings/SettingsEdit', |
There was a problem hiding this comment.
Please revert this change to title so the SettingsEdit stories are next to SettingsView in the Storybook menu:
| title: 'Modules/ReaderRevenueManager/Components/Settings/SettingsEdit', | |
| title: 'Modules/ReaderRevenueManager/Settings/SettingsEdit', |
| }, | ||
| }; | ||
|
|
||
| export const PublicationSelectedWithOnboardingStateNotice = Template.bind( {} ); |
There was a problem hiding this comment.
Hmm... You could rename this to PublicationSelectedPendingVerification, and create a similar story with a publication in the ONBOARDING_ACTION_REQUIRED state called PublicationSelectedActionRequired.
Then, you could remove the scenarios (but not the stories) for the PublicationOnboardingStateNotice component, because they will both effectively be covered here.
I realise it means backing out a bit of work that was just added, but the net result will be better coverage for the SettingsEdit component, and one less scenario in our VRT suite, which we are always looking to make more efficient.
| muteFetch( settingsEndpoint ); | ||
| muteFetch( publicationsEndpoint ); | ||
| muteFetch( listModulesEndpoint ); |
There was a problem hiding this comment.
Our JS tests should generally not need to know about the underlying REST endpoints, unless they are explicitly testing the network behaviour.
Please can you rewrite this test to populate the relevant data rather than muting these endpoints. For example, rather than muting settingsEndpoint, you can restore the following call:
registry
.dispatch( MODULES_READER_REVENUE_MANAGER )
.receiveGetSettings( {} );We're already populating the list of publications via receiveGetPublications() in the setup, so muting publicationsEndpoint is redundant and can simply be removed. The same applies to the listModulesEndpoint, the modules are already being populated via provideModules().
| muteFetch( settingsEndpoint ); | |
| muteFetch( publicationsEndpoint ); | |
| muteFetch( listModulesEndpoint ); |
| @@ -72,6 +72,7 @@ protected function get_default() { | |||
| 'publicationID' => '', | |||
| 'publicationOnboardingState' => '', | |||
| 'publicationOnboardingStateLastSyncedAtMs' => 0, | |||
| 'ownerID' => 0, | |||
There was a problem hiding this comment.
This is very minor, but please can you move ownerID to the top of the list of fields to be consistent with settings for other modules. For example:
site-kit-wp/includes/Modules/AdSense/Settings.php
Lines 190 to 192 in e112780
| await waitForRegistry(); | ||
|
|
||
| // Ensure publication select is rendered. | ||
| expect( container.querySelector( '.mdc-select' ) ).toBeInTheDocument(); |
There was a problem hiding this comment.
We try to follow the general principles behind the Testing Library / RTL, whereby we interact with the components "in the way the user would use it" as much as possible. This means we prefer the getByRole(), getByText() etc. queries to querying the DOM using selectors.
In this case, we can use getByRole() instead of querying for .mdc-select:
| expect( container.querySelector( '.mdc-select' ) ).toBeInTheDocument(); | |
| expect( getByRole( 'menu', { hidden: true } ) ).toBeInTheDocument(); |
| container.querySelector( | ||
| '.googlesitekit-publication-onboarding-state-notice' | ||
| ) |
There was a problem hiding this comment.
As above:
| container.querySelector( | |
| '.googlesitekit-publication-onboarding-state-notice' | |
| ) | |
| getByText( | |
| 'Your publication requires further setup in Reader Revenue Manager' | |
| ) |
| const { container, waitForRegistry } = render( <SettingsEdit />, { | ||
| registry, | ||
| } ); |
There was a problem hiding this comment.
You'll want to retrieve getByRole and getByText here to address the points below:
| const { container, waitForRegistry } = render( <SettingsEdit />, { | |
| registry, | |
| } ); | |
| const { waitForRegistry, getByRole, getByText } = render( | |
| <SettingsEdit />, | |
| { | |
| registry, | |
| } | |
| ); |
| } ); | ||
|
|
||
| if ( isDoingSubmitChanges || undefined === hasModuleAccess ) { | ||
| return <ProgressBar smallHeight={ 80 } desktopHeight={ 88 } medium />; |
There was a problem hiding this comment.
This can be rendered without props to be consistent with the other SettingsEdit components:
| return <ProgressBar smallHeight={ 80 } desktopHeight={ 88 } medium />; | |
| return <ProgressBar />; |
Summary
Addresses issue:
Relevant technical choices
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