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

Dependency version upgrades, not API 35 yet #1719

Merged
merged 5 commits into from
Dec 10, 2024
Merged

Conversation

keyboardsurfer
Copy link
Member

No description provided.

gradle/libs.versions.toml Outdated Show resolved Hide resolved
Copy link

@riggaroo riggaroo left a comment

Choose a reason for hiding this comment

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

Looks like the top app bar changed and i dont know if we want that?

@riggaroo
Copy link

riggaroo commented Dec 9, 2024

It seems to have removed the cog icons and search.

@keyboardsurfer keyboardsurfer force-pushed the bw/versionUpgrades branch 4 times, most recently from 28c0487 to 1329f6e Compare December 9, 2024 16:32
Change-Id: Ibed7889a3389ab6044e00a185158f36ce857b6b2
@keyboardsurfer keyboardsurfer changed the title Upgrade compileSdk to 35, dependency version upgrades Dependency version upgrades, not API 35 yet Dec 10, 2024
@keyboardsurfer
Copy link
Member Author

The top app bar changes were introduced with the upgrade to API 35 and are only visible when running roborazzi. For this maintenance effort it's out of scope for me to dive into roborazzi and address this.

Instead I reverted back to API 35 for target and compile Sdk version.

@takahirom FYI

@keyboardsurfer keyboardsurfer dismissed riggaroo’s stale review December 10, 2024 11:15

Changes addressed but can't be resolved due to force push on branch

@takahirom
Copy link
Contributor

takahirom commented Dec 10, 2024

Thank you for reporting this 👀
I think SDK 35 doesn't provide padding for the ActionBar, and since HiltComponentActivity in NowInAndroid includes the ActionBar, it ends up overlapping with the content.

image

Roborazzi is not using view.draw(); instead, we capture window rendering. (I think this approach might involve some tradeoffs, such as fidelity and issues like this.)
Overlapping UI could also cause issues with other test APIs, such as clicking, so it might be good to avoid overlapping.

I believe simply setting windowActionBar = false could resolve this issue.

I'm checking if this approach works:
takahirom#4
takahirom#5
takahirom#6

@keyboardsurfer
Copy link
Member Author

Thanks for checking this @takahirom. It seems like takahirom#6 is the approach we should take for Now in Android.
@dturner WDYT?

@keyboardsurfer keyboardsurfer force-pushed the bw/versionUpgrades branch 2 times, most recently from a85c194 to 2cbaffd Compare December 10, 2024 14:09
Change-Id: Ib4a1f9bef1dd3e1736907479125d0abb5b22e9bd
@dturner
Copy link
Collaborator

dturner commented Dec 10, 2024

It'd be more conventional to set the theme in the manifest i.e. add android:theme="@android:style/Theme.Material.Light.NoActionBar
to the ui-test-hilt-manifest module's AndroidManifest.xml

@keyboardsurfer
Copy link
Member Author

While @takahirom's approach seems to work for light theme, there are still issues around theming with API 35 when using roborazzi. This change is out of scope for this PR. So we're sticking with 34 for now.

Change-Id: Ia2c6a52e289285a7a78336fa4250fa6a04ccb5c4
Copy link
Collaborator

@dturner dturner left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. Good to merge once the build and instrumented tests pass.

@takahirom
Copy link
Contributor

takahirom commented Dec 10, 2024

It seems that val composeTestRule = createComposeRule() has the same issue in a simple library.
I was able to reproduce it in an emulator test (instrumentation test) as well, so I'm thinking of filing this issue on Issue Tracker tomorrow or the day after.
image

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