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.

Sign up for GitHub

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

Jump to bottom

Migrate PluginDetailActivity to ViewBinding #20968

Merged
merged 5 commits into from
Jun 21, 2024
Merged

Migrate PluginDetailActivity to ViewBinding #20968

merged 5 commits into from
Jun 21, 2024

Conversation

neeldoshii
Copy link
Contributor

Description

This pull request migrates the PluginDetailActivity to use ViewBinding, improving type safety and eliminating the need for findViewById calls.

Fixes

Screenshots/ Video

// TODO

Steps to reproduce

  1. Ensure that you are on Wasabi build variant.
  2. Click on my Site
  3. In manage --> Click on Plugins
  4. Click on any plugin
  5. Play with the plugin :D

PR Submission Checklist:

  • I have completed the Regression Notes.
  • 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.

Testing Checklist (strike-out the not-applying and unnecessary ones):

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • Talkback.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • Large and small screen sizes. (Tablet and smaller phones)
  • Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

@neeldoshii neeldoshii marked this pull request as draft June 11, 2024 06:02
@neeldoshii neeldoshii force-pushed the Migrate-`PluginDetailActivity`-to-ViewBinding branch from 5efa4d6 to 8a13344 Compare June 11, 2024 06:14
@neeldoshii neeldoshii marked this pull request as ready for review June 11, 2024 06:14
neeldoshii
neeldoshii commented Jun 11, 2024
View reviewed changes
WordPress/src/main/java/org/wordpress/android/ui/plugins/PluginDetailActivity.java Outdated
@@ -865,7 +808,7 @@ private void cancelDomainCreditsCheckProgressDialog() {
mCheckingDomainCreditsProgressDialog.cancel();
}
}

@SuppressWarnings("deprecation") // Progress Dialog Deprecation on API 26
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewer Note :

Do we have any issue which says of migrating the deprecated Progress Dialog to ProgressBar/Notification? If so then should I comment issue number over here for ease in understanding the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverting this due to not related to context of the PR.

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jun 13, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@neeldoshii
Copy link
Contributor Author

CC @ParaskP7 request to review once you are free.

@ParaskP7 ParaskP7 self-requested a review June 20, 2024 12:33
@ParaskP7 ParaskP7 added this to the Future milestone Jun 20, 2024
@ParaskP7 ParaskP7 mentioned this pull request Jun 20, 2024
Merged
ParaskP7
ParaskP7 approved these changes Jun 20, 2024
View reviewed changes
Copy link
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

👋 @neeldoshii !

Thank you so much for your contribution to JP/WPAndroid with this PR! 🥇

I have reviewed and tested this PR as per the instructions, along with triggering CI on it via a draft PR, which I just pushed on the main repo, everything works as expected, good job! 🌟

I have left few minor (🔍) comments for you to consider. I am going to approve this PR anyway, since none is blocking. I am NOT going to merge this PR yet to give you some time to apply any of my suggestions. However, feel free to ignore them and merge the PR yourself.

WordPress/src/main/res/layout/plugin_detail_activity.xml Show resolved Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/plugins/PluginDetailActivity.java Outdated Show resolved Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/plugins/PluginDetailActivity.java Outdated Show resolved Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/plugins/PluginDetailActivity.java Outdated Show resolved Hide resolved
@neeldoshii
Copy link
Contributor Author

Thank you for reviewing @ParaskP7 🚀. I have applied the requested changes! 😅

I am NOT going to merge this PR yet to give you some time to apply any of my suggestions. However, feel free to ignore them and merge the PR yourself.

I don't think I have the permission to merge the PR. 😅

@ParaskP7
Copy link
Contributor

Thanks for applying my suggestions @neeldoshii ! 🥇

I don't think I have the permission to merge the PR. 😅

Yea, contributors don't have the permission to merge, but also, I first need to re-trigger CI on the changes you made. Let me do this work and merge this if everything goes well! 👍

@ParaskP7 ParaskP7 merged commit 9a3067c into wordpress-mobile:trunk Jun 21, 2024
21 checks passed
@ParaskP7
Copy link
Contributor

🎉 @neeldoshii , another contribution in, thanks so much for all your work! 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ParaskP7 ParaskP7 ParaskP7 approved these changes

Assignees
No one assigned
Labels
Core Tech Debt
Projects
None yet
Milestone
Future
Development

Successfully merging this pull request may close these issues.
2 participants
@neeldoshii @ParaskP7