Skip to content
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

Implement UI for new AdSense setup main components #4762

Closed
felixarntz opened this issue Feb 1, 2022 · 6 comments
Closed

Implement UI for new AdSense setup main components #4762

felixarntz opened this issue Feb 1, 2022 · 6 comments
Labels
Good First Issue Good first issue for new engineers Module: AdSense Google AdSense module related issues P0 High priority QA: Eng Requires specialized QA by an engineer Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature

Comments

@felixarntz
Copy link
Member

felixarntz commented Feb 1, 2022

This issue is for actually implementing the UI (and a bit off extra logic) for the enhanced AdSense setup flow main components that were introduced in #4758.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The SetupCreateAccount component from Scaffold (logic for) new AdSense setup main components #4758 should have its UI implemented as follows:
    • It should be almost an exact copy of the existing (V1) SetupAccountCreate component (not reusing the existing component), with just a few wording adjustments:
      • The main body text (i.e. the one above the user profile info) should be replaced with: Once you create your account, Site Kit will place AdSense code on every page across your site. This means your site will be automatically optimized to help you earn money from your content.
      • The CTA button text should be replaced with: Create AdSense account (same as today, but lowercase "a")
  • The SetupSelectAccount component from Scaffold (logic for) new AdSense setup main components #4758 should have its UI implemented as follows:
    • It should be an exact copy of the existing (V1) SetupAccountSelect component (not reusing the existing component).
  • The UI implemented here can be tested via the Storybook stories introduced in Scaffold (logic for) new AdSense setup main components #4758, and it should be ensured it looks correct there (in case e.g. any additional data needs to be bootstrapped in the Storybook configuration).

Implementation Brief

  • Using assets/js/modules/adsense/components/setup/v2/SetupCreateAccount.js,
    • Copy over the code from assets/js/modules/adsense/components/setup/SetupAccountCreate.js to the file with the text changes as per the AC.
  • Using assets/js/modules/adsense/components/setup/v2/SetupSelectAccount.js,
    • Copy over the code from assets/js/modules/adsense/components/setup/SetupSelectAccount.js to the file.
  • Update assets/js/modules/adsense/components/setup/v2/SetupMain.stories.js to include stories where above components are rendered.

Test Coverage

  • No new tests to be added.

QA Brief

  • Verify the changes met AC and IB.
  • Verify the stories render SetupCreateAccount and SetupSelectAccount components as expected.

Changelog entry

  • Add UI for new AdSense components.
@felixarntz felixarntz added P0 High priority Type: Enhancement Improvement of an existing feature Module: AdSense Google AdSense module related issues labels Feb 1, 2022
@felixarntz felixarntz self-assigned this Feb 1, 2022
@felixarntz felixarntz removed their assignment Feb 22, 2022
@felixarntz felixarntz added the Good First Issue Good first issue for new engineers label Feb 22, 2022
@asvinb asvinb self-assigned this Feb 28, 2022
@asvinb
Copy link
Collaborator

asvinb commented Mar 1, 2022

@felixarntz Just some clarification when the AC says:

not reusing the existing component
I've assumed that it means not updating assets/js/modules/adsense/components/setup/v2/SetupCreateAccount.js and assets/js/modules/adsense/components/setup/v2/SetupSelectAccount.js but to copy the code over.

Let me know if am correct in my assumption.

@asvinb asvinb assigned felixarntz and unassigned asvinb Mar 1, 2022
@felixarntz
Copy link
Member Author

@asvinb Your IB is correct - my point was that the code should be copied over from the V1 components, and the V2 components should not use the V1 components (for example the V2 SetupCreateAccount component should not render/return the V1 <SetupAccountCreate /> in any case).

IB ✅

@felixarntz felixarntz removed their assignment Mar 1, 2022
@hussain-t hussain-t assigned hussain-t and unassigned hussain-t Mar 15, 2022
@hussain-t hussain-t self-assigned this Mar 23, 2022
@hussain-t hussain-t removed their assignment Mar 28, 2022
@FlicHollis FlicHollis added the Rollover Issues which role over to the next sprint label Mar 28, 2022
@asvinb asvinb assigned asvinb and unassigned asvinb Mar 28, 2022
@tofumatt tofumatt assigned tofumatt and hussain-t and unassigned tofumatt Mar 28, 2022
@hussain-t hussain-t assigned tofumatt and unassigned hussain-t Mar 30, 2022
@tofumatt tofumatt removed their assignment Apr 5, 2022
@wpdarren wpdarren self-assigned this Apr 6, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Apr 11, 2022

QA Update: ❌

@hussain-t As you can see from the screenshot below the CTA button text is in uppercase. According to the AC, it probably shouldn't be. It does appear as uppercase on V1, but would like to check.

The CTA button text should be replaced with: Create AdSense account (same as today, but lowercase "a")

I also feel this ticket should also be QA:Eng because of the IB checks included in the QAB. Will add the label once it passes our initial testing.

image

@wpdarren wpdarren removed their assignment Apr 11, 2022
@hussain-t
Copy link
Collaborator

As you can see from the screenshot below the CTA button text is in uppercase. According to the AC, it probably shouldn't be. It does appear as uppercase on V1, but would like to check.

@wpdarren, all the buttons should be in uppercase to keep it consistent with the current styles. cc: @asvinb

@hussain-t hussain-t assigned wpdarren and unassigned hussain-t Apr 11, 2022
@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

  • Main body text is be replaced with:
    Once you create your account, Site Kit will place AdSense code on every page across your site. This means your site will be automatically optimized to help you earn money from your content.

image

Have added QA:Eng label on for testing the IB.

@wpdarren wpdarren removed their assignment Apr 11, 2022
@wpdarren wpdarren added the QA: Eng Requires specialized QA by an engineer label Apr 11, 2022
@techanvil techanvil self-assigned this Apr 20, 2022
@techanvil
Copy link
Collaborator

QA ✅

QA:Eng:

  • Confirmed the IB has been implemented as specced.
  • Nb, the relevant stories had previously been created in the PR Enhancement/4758 new adsense setup #4959 along with placeholder components, so no Storybook changes were here, these placeholders being fleshed out in the present issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers Module: AdSense Google AdSense module related issues P0 High priority QA: Eng Requires specialized QA by an engineer Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants