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

Remove Google domains promotional card from the dashboard #22216

Merged
merged 5 commits into from
Dec 19, 2023

Conversation

hassaanelgarem
Copy link
Contributor

Description

This PR disables the Google domains card in the dashboard.

Testing Instructions

  1. Install the Jetpack app
  2. Scroll through the dashboard
  3. Make sure the Google Domains card is not visible

Regression Notes

  1. Potential unintended areas of impact
    N/A
  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    N/A
  3. What automated tests I added (or what prevented me from doing so)
    N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 13, 2023

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr22216-860bda0
Version23.9
Bundle IDorg.wordpress.alpha
Commit860bda0
App Center BuildWPiOS - One-Offs #8170
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 13, 2023

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr22216-860bda0
Version23.9
Bundle IDcom.jetpack.alpha
Commit860bda0
App Center Buildjetpack-installable-builds #7190
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@alpavanoglu
Copy link
Contributor

Will we not remove the DashboardGoogleDomainsCardView and related UI code?

@hassaanelgarem
Copy link
Contributor Author

@alpavanoglu We should, yes. I'd prefer us not to do it straight away though, just to be safe. I'm preparing a draft PR now that removes the unneeded code and also refactors BlogDashboardPersonalizationService a bit to improve its testability, cause I found out we have some tests that directly depend on the Google Domains card being there, which is not great.

@alpavanoglu
Copy link
Contributor

@hassaanelgarem sounds good. I think we can open/merge the follow up PR whenever it is ready as if in time, we need the card again, we can find it through the git history. It doesn't really vanish after all if that's what you have meant.

case .domainFocus:
return true
case .googleDomainsCard:
return false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional:

I think we could add a comment here saying that the functionality will be removed with a follow up pr and/or link the draft PR. If you think that it isn't necessary as you are already working on it, that's also fine.

@alpavanoglu
Copy link
Contributor

After merging from trunk the CI should pass now.

@hassaanelgarem hassaanelgarem requested a review from a team as a code owner December 18, 2023 22:46
@hassaanelgarem hassaanelgarem merged commit fa1fd85 into trunk Dec 19, 2023
22 of 23 checks passed
@hassaanelgarem hassaanelgarem deleted the task/remove-google-domains-card branch December 19, 2023 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants