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

Addresses lint issues in AGP 7.2.1 update #16839

Merged

Conversation

oguzkocer
Copy link
Contributor

@oguzkocer oguzkocer commented Jun 29, 2022

This PR addresses the lint issues due to AGP 7.2.1 update in #16834.

  • 84598d0 removes the unused TextDrawable widget, so we don't even need to fix the lint issue 🙈
  • d1eeb1e removes the custom divider and uses the built in DividerItemDecoration instead. Funnily enough I worked on the people list 6+ years ago and DividerItemDecoration was not available back then (was added a few months after) so we had to build it ourselves. The difference between the regular divider and the custom divider is that the regular divider covers the whole width whereas the custom divider had some space before and after the line. See the screenshots below for comparison. Since we are using the regular divider in a lot of newer screens, this change should bring the design of people list closer to the rest of the app so it's a win-win situation.
  • 9988ff8 addresses Obsolete SDK_INT version checks lint error.

Before:

(Full name and display name are removed and the screenshot was cut in half to make it easier to see)

After:

(Full name and display name are removed and the screenshot was cut in half to make it easier to see)

To test:

  • Smoke test the app
  • Checkout the People page from "My Site" -> "People" and verify that the divider looks OK

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 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.

@oguzkocer oguzkocer added the Lint label Jun 29, 2022
@oguzkocer oguzkocer added this to the 20.3 milestone Jun 29, 2022
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jun 29, 2022

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@oguzkocer oguzkocer changed the base branch from trunk to upgrade-gradle-to-7.4.2-agp-to-7.2.1 June 29, 2022 16:44
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 29, 2022

You can test the WordPress changes on this Pull Request by downloading an installable build (wordpress-installable-build-pr16839-0e12fd6.apk), or scanning this QR code:

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 29, 2022

You can test the Jetpack changes on this Pull Request by downloading an installable build (jetpack-installable-build-pr16839-0e12fd6.apk), or scanning this QR code:

@oguzkocer oguzkocer force-pushed the agp-7.2.1/lint-issues branch from 7519752 to 9988ff8 Compare June 29, 2022 17:04
@oguzkocer oguzkocer marked this pull request as ready for review July 4, 2022 15:20
@oguzkocer oguzkocer requested a review from ParaskP7 July 4, 2022 15:20
@ParaskP7 ParaskP7 self-assigned this Jul 5, 2022
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.

👋 @oguzkocer !

I have reviewed and tested this PR as per the instructions, everything works as expected, good job! 🌟


I have left one suggestion (💡) 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.

@ParaskP7
Copy link
Contributor

ParaskP7 commented Jul 5, 2022

Funnily enough I worked on the people list 6+ years ago and DividerItemDecoration was not available back then (was added a few months after) so we had to build it ourselves.

  1. It feels nice to replace code you've written in the past with a much simpler version of it, isn't it! 😄
  2. I find awesome the fact that you keep working and applying love to a codebase for more than 6 years now, kudos! ❤️

@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@ParaskP7 ParaskP7 merged commit 5e2660c into upgrade-gradle-to-7.4.2-agp-to-7.2.1 Jul 6, 2022
@ParaskP7 ParaskP7 deleted the agp-7.2.1/lint-issues branch July 6, 2022 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants