Skip to content

Conversation

@android-dev-lxl
Copy link
Collaborator

@android-dev-lxl android-dev-lxl commented Jun 29, 2023

This is the reference PR from Meghan: Woof edge to edge

Changes in this PR:

  • Simplified the code a bit
  • Removed the code for lower SDK versions since the minimum SDK we support is 24
  • Deprecated APIs have been replaced with alternate APIs
  • Tested the UI on multiple emulator versions
  • Fixed content padding issue

I would appreciate it if you could take a look at these changes and let me know.

Copy link

@MagicalMeghan MagicalMeghan left a comment

Choose a reason for hiding this comment

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

LGTM for e2e in Theme.kt file

Copy link

@johnshea johnshea left a comment

Choose a reason for hiding this comment

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

LGTM with two quick fixes needed.

@johnshea johnshea self-requested a review August 29, 2023 13:40
Copy link

@johnshea johnshea left a comment

Choose a reason for hiding this comment

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

Update gradle wrapper to latest version. Currently 8.2
Update kotlinCompilerExtensionVersion to the latest.
Update BOM to the latest.
Project build.gradle.kts - Update all plugins to the latest versions

Type.kt - ln 31 - insert blank line to push comment down
Type.kt - ln 33 & ln 41 - remove blank lines for consistency

Choose a reason for hiding this comment

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

Should the MaterialTheme shape value be used here?
Screenshot 2023-08-29 at 10 10 17 AM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this was already reviewed by SMEs

Choose a reason for hiding this comment

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

There are multiple occurences of 72.dp and 8.dp.

Can these be moved to the dimens.xml?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is no dimens.xml for this project.

Copy link

@johnshea johnshea left a comment

Choose a reason for hiding this comment

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

One nit - build.gradle.kts - You updated to a newer version but didn't update to the latest one.

@android-dev-lxl android-dev-lxl merged commit ac83068 into main Sep 11, 2023
@android-dev-lxl android-dev-lxl deleted the main-edge-to-edge branch September 11, 2023 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants