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

Finish m2 to m3 conversion #21583

Merged
merged 36 commits into from
Jan 15, 2025
Merged

Finish m2 to m3 conversion #21583

merged 36 commits into from
Jan 15, 2025

Conversation

nbradbury
Copy link
Contributor

@nbradbury nbradbury commented Jan 14, 2025

Closes #21065 - As mentioned on pbArwn-6XC-p2#comment-8241, the conversion from Material 2 (M2) to Material 3 (M3) was incomplete. This PR addresses all the remaining issues.

As part of this, I changed lint.xml so that UsingMaterialAndMaterial3Libraries is treated as an error rather than a warning. This way lint will fail if we accidentally introduce M2 components in the future.

Testing isn't straightforward because this affected so many areas, none of which were really large enough to warrant separate PRs. Also, many of the changes involved reusable components such MainTopAppBar which are used throughout the app. My recommendation is to first ensure lint passes, then smoke test the app and verify nothing looks out of whack.

That said, the areas most impacted by these changes - and therefore needing the most testing - are:

  • Login
  • Social sharing
  • Post resolution conflict

That last one is tricky to test, so here's a screenshot of the updated conflict dialog:

post-resolution

Social sharing is likewise tricky to test because setting up social connections appears to be broken.

@nbradbury nbradbury added Tech Debt Material3 Related to updating to Material 3 labels Jan 14, 2025
@dangermattic
Copy link
Collaborator

dangermattic commented Jan 14, 2025

2 Warnings
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 14, 2025

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr21583-11a5fd9
Commit11a5fd9
Direct Downloadjetpack-prototype-build-pr21583-11a5fd9.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 14, 2025

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr21583-11a5fd9
Commit11a5fd9
Direct Downloadwordpress-prototype-build-pr21583-11a5fd9.apk
Note: Google Login is not supported on these builds.

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 0% with 32 lines in your changes missing coverage. Please review.

Project coverage is 39.48%. Comparing base (e72074a) to head (11a5fd9).
Report is 37 commits behind head on trunk.

Files with missing lines Patch % Lines
...ordpress/android/ui/posts/PostResolutionOverlay.kt 0.00% 18 Missing ⚠️
...osts/EditPostSettingsJetpackSocialNoConnections.kt 0.00% 5 Missing ⚠️
...posts/EditPostJetpackSocialConnectionsContainer.kt 0.00% 3 Missing ⚠️
...ts/EditPostSettingsJetpackSocialSharesContainer.kt 0.00% 3 Missing ⚠️
...d/ui/posts/EditPostJetpackSocialConnectionsList.kt 0.00% 2 Missing ⚠️
...dpress/android/ui/barcodescanner/BarcodeScanner.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            trunk   #21583   +/-   ##
=======================================
  Coverage   39.48%   39.48%           
=======================================
  Files        2117     2117           
  Lines       99464    99464           
  Branches    15296    15296           
=======================================
  Hits        39269    39269           
  Misses      56714    56714           
  Partials     3481     3481           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nbradbury nbradbury marked this pull request as ready for review January 15, 2025 15:46

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

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

The changes look sound to me. I tested logging in and explored various aspects of publishing. I noted a few oddities captured in screenshots below, but they appear unrelated to these changes.

Oddity screenshots

Screenshot_20250115_133755_Jetpack Pre-Alpha
Screenshot_20250115_133848_Jetpack Pre-Alpha
Screenshot_20250115_133934_Jetpack Pre-Alpha
Screenshot_20250115_134006_Jetpack Pre-Alpha

@nbradbury
Copy link
Contributor Author

Thanks for the review @dcalhoun - I know this was a tedious one! I checked and all of those oddities exist in trunk, so I think we're good to go.

@nbradbury nbradbury merged commit 36f61cd into trunk Jan 15, 2025
22 checks passed
@nbradbury nbradbury deleted the issue/finish-m2-to-m3-conversion branch January 15, 2025 19:48
@@ -79,7 +79,7 @@
<issue id="IconDensities" severity="warning" /> <!-- fluxc-->
<!-- INTEROPERABILITY -->
<!-- TODO: https://github.com/wordpress-mobile/WordPress-Android/issues/21065 -->
<issue id="UsingMaterialAndMaterial3Libraries" severity="warning" />
<issue id="UsingMaterialAndMaterial3Libraries" severity="error" />
Copy link
Contributor

Choose a reason for hiding this comment

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

👋 @nbradbury and awesome job with all this! ❤️

About switching from warning to error, just because this UsingMaterialAndMaterial3Libraries check is of Warning severity by default, but we are anyway treating all warning as errors (WordPress/build.gradle#L317), I would recommend deleting this whole configuration and its comment (#21065), doing that we will end-up with the same result.

Copy link

sentry-io bot commented Jan 25, 2025

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ ApplicationNotResponding: Background ANR org.wordpress.android.WordPress in newImageLoader View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Material3 Related to updating to Material 3 Tech Debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolve UsingMaterialAndMaterial3Libraries errors
5 participants