-
Notifications
You must be signed in to change notification settings - Fork 291
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
Scaffold RRM module setup flow #8800
Comments
Note: Added a new point in the ACs:
Back to IB again. |
Excellent IB, thanks @ankitrox ! Please take a look at my comments below:
Let's change the object key to be something more meaningful, such as
Instead of using the number of publications to show
The "Create publication" CTA inside However, the
This bit is not necessary, as mentioned above, we should always render
Let's also set the
In order to save settings in the setup/settings screens, we should ideally use the Please let me know if you have any questions on the above, thank you! |
Thanks for reviewing the IB @nfmohit and your valuable feedback. I've made the changes as per the suggestions. Over to you for re-review. Thanks again! |
Thank you for the iteration here, @ankitrox ! This looks good to me. Note: I've removed the requirement to set IB ✅ |
QA Update
|
Thank you for sharing your observations, @kelvinballoo ! Regarding Item 1, it looks like the QAB and implementation do not match. @kuasha420 I'd like to have your opinion here. We only refresh the publications in the case where there isn't any publication already selected (see ACs for #8839). In this scenario, since a publication is already selected, the publications are not refreshed. Do you think we should also refresh in this case? @kelvinballoo Regarding Item 2, the publication will be selected based on the condition here, i.e. if you have two publications, it will select the publication that has onboarding completed. If none of the publications have completed onboarding, it will just select the first available publication. |
Hi @nfmohit, since there's a CTA to create new publication, I think we SHOULD refresh the list even if there are existing publication on tab refocus similar to how it's done for no publication scenario, otherwise it is bit of a UX miss. Thank you. |
Agreed, thank you @kuasha420. @kelvinballoo I'm sending this back to Execution to address Item 1. Thanks! |
Back to you @kelvinballoo for another pass 👍 |
QA Update
|
Thank you for sharing your observations, @kelvinballoo!
Please take a look at my screencast below: 1.mp4As you can see in my screencast, I did not need to click on the page for the reload to start. As soon as I went back to the RRM setup tab, the reload started. Could you try again?
Apologies for the confusion here. It is not mandatory that the latest publication will be selected. The publication auto-select will still happen based on the condition here, i.e. if you have more than one publication, it will select the publication that has onboarding completed. If none of the publications have completed onboarding, it will just select the first available publication in the list. Please let me know if you have any other questions, thank you! |
QA Update
|
Hi @kelvinballoo . This is expected for the setup form. The behaviour that you linked to is only valid for the "no publications available" scenario, however, that has been changed as well. From now on, the automatic refresh will only happen if the user clicks on "Create new publication", goes to a different tab, and returns to the setup tab after 15 seconds. |
QA Update ✅Thanks @nfmohit This is thus verified good:
Available publications in account scenario ✅
Moving ticket to Approval |
Feature Description
The Reader Revenue Manager module setup flow has different states based on the number of publications the user has. This issue should handle setting the module setup screen and placeholder components for each state.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
<PublicationCreate>
component being added as part of #8836).<PublicationCreate>
component, they shouldn't be shown the content below even though Site Kit detects that publications exist later. This is crucial to ensure that the two steps in the<PublicationCreate>
component that appear based on the availability of publications function correctly. This can be done by using a new form key in thecore/forms
data store.<PublicationSelect>
component being added as part of #8837).findMatchedPublication()
action being added as part of #8795).<PublicationOnboardingStateNotice>
component being added as part of #8838).Implementation Brief
READER_REVENUE_MANAGER_SETUP_FORM
with valuereaderRevenueManagerSetupFormData
inassets/js/modules/reader-revenue-manager/datastore/constants.js
.SHOW_PUBLICATION_CREATE_FORM
with valueshowPublicationCreateForm
inassets/js/modules/reader-revenue-manager/datastore/constants.js
.assets/js/modules/reader-revenue-manager/components/setup/SetupMain.js
finishSetup
prop which is a function.useSelect
hook andgetPublications
selector from RRM store.getPublications
selector is not resolved yet, display theProgressBar
component.setValues
function fromcore/forms
store. Pass form name asREADER_REVENUE_MANAGER_SETUP_FORM
and value as an object as followinglet
variableviewComponent
which will hold the component that needs to be displayed.PublicationCreate
component toviewComponent
if value ofSHOW_PUBLICATION_CREATE_FORM
istrue
. "Create Publication" CTA would be displayed as part of Implement<PublicationCreate>
component #8836, theonCompleteSetup
for the CTA should be passed which should dispatch thesubmitChanges
action and call thefinishSetup
function.PublicationCreate
component will render the next step as mentioned below.PublicationSelect
component. Wrap the text and dropdown component in a div with class namegooglesitekit-settings-module__fields-group
. Refer this component from ads module for similar structure, just replaceTextField
withPublicationSelect
component.PublicationSelect
component. UsegetServiceURL
selector added in Implement RRMgetServiceURL()
selector #8848 to get the link to publication center which will get opened in a new tab.findMatchedPublication
action to find the suitable publication in case there is no publication ID available already and multiple publications are available. If the publication is returned by the action, set publication ID, onboarding state and sync timestamp usingsetPublicationID
andsetPublicationOnboardingState
actions respectively, so thatPublicationSelect
component can retrieve it and display it as selected one.Complete setup
should dispatchsubmitChanges
on RRM module store and call thefinishSetup
function.Test Coverage
QA Brief
rrmModule
feature flag.No publications in account scenario
Available publications in account scenario
Note: You may notice some differences in styles compared to the Figma designs, specifically regarding the introductory "Site Kit will connect your existing publication" text and spacing between elements. This is expected to ensure consistency with other modules' setup flows. See discussion here.
Changelog entry
The text was updated successfully, but these errors were encountered: