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

Adding support for Compose assets and reference assets #537

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Adding support for Compose assets and reference assets #537

wants to merge 15 commits into from

Conversation

A1shK
Copy link
Contributor

@A1shK A1shK commented Nov 4, 2024 •
edited
Loading

This PR adds:

  1. Support for creating Android assets using Jetpack Compose (Provide the bridge to Android Player for Jetpack Compose users #89)
    2. Adding reference assets using Jetpack Compose - Collection and Badge (Create Jetpack Compose Reference Assets #90)

Relevant files:

  1. Documentation
  2. ComposableAsset - base compose asset class
  3. Collection and badge - examples of assets that extend ComposableAsset
  4. Tests for these assets that use ComposeUITest (compose equivalent of AssetUITest) - badge and collection
  5. Dependency files - the right combination of dependencies took a lot of trial and erro due to the restriction on the kotlin version. The max compose version we can be on is 1.2.0 thats compatible with kotlin 1.7.20.
  6. AndroidManifest changes - reference-asset needed the minSDK versions and both AndroidManifests needed some code to remove a conflict with the InitializationProvider
  7. Hook that adds support for Compose theming. I tried a few different combinations of hooks. To avoid having to make @Composable calls, I ended up working directly with ProvidedValues.

Change Type (required)

Indicate the type of change your pull request is:

  • patch
  • minor
  • major

Does your PR have any documentation updates?

  • Updated docs
  • No Update needed
  • Unable to update docs

@codecov Codecov
Copy link

codecov bot commented Nov 4, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.76%. Comparing base (cfdf872) to head (5bdf6d4).

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #537   +/-   ##
=======================================
  Coverage   89.75%   89.76%           
=======================================
  Files         331      331           
  Lines       19836    19836           
  Branches     1949     1949           
=======================================
+ Hits        17804    17805    +1     
+ Misses       2018     2017    -1     
  Partials       14       14

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

@A1shK A1shK force-pushed the composable-asset branch 2 times, most recently from 9a1e5cf to 043d251 Compare November 7, 2024 16:42
@nancywu1
Copy link
Contributor

nancywu1 commented Dec 3, 2024

/canary

3 similar comments
@KetanReddy
Copy link
Member

/canary

@nancywu1
Copy link
Contributor

nancywu1 commented Dec 6, 2024

/canary

@brocollie08
Copy link
Contributor

/canary

@A1shK A1shK force-pushed the composable-asset branch from d4b7917 to ab8d9c0 Compare January 5, 2025 16:26
@A1shK A1shK changed the title [WIP] Adding support for Compose assets and reference assets Adding support for Compose assets and reference assets Jan 13, 2025
@A1shK A1shK marked this pull request as ready for review January 13, 2025 14:58
A1shK
A1shK commented Jan 13, 2025
View reviewed changes
android/demo/README.md
@@ -9,8 +9,7 @@ It's been tested on Andriod Studio Chipmunk(2021.2.1) and Android Studio Giraffe
Assuming you have read the [requirements on the root contributing guide](https://github.com/player-ui/player/blob/main/CONTRIBUTING.md).

1. Once you have Android Studio installed, you will need to go to tools->SDK Manager->SDK Platforms.
1. Make sure you have **only** the following SDK installed: Android API 32.
*If you are using Android Giraffe, you may need to click on show package details and it will be under Android12L. (Android SDK Platrform 32)*
1. Make sure you have **only** the following SDK installed: Android API 33.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is needed but I couldnt get it to build with API 32

android/player/src/main/java/com/intuit/playerui/android/asset/RenderableAsset.kt
@@ -186,7 +186,7 @@ public constructor(public val assetContext: AssetContext) : NodeWrapper {
.render()

/** Render a [View] with specific [styles] */
public fun RenderableAsset.render(@StyleRes styles: Styles): View = assetContext
public fun RenderableAsset.render(@StyleRes styles: Styles?): View = assetContext
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated these to avoid have to call multiple render implementations here

Any concerns?

@KetanReddy KetanReddy added the minor Increment the minor version when merged label Jan 17, 2025
KetanReddy
KetanReddy reviewed Jan 17, 2025
View reviewed changes
MODULE.bazel
rules_kotlin_extensions.kotlinc_version(
sha256 = "9db4b467743c1aea8a21c08e1c286bc2aeb93f14c7ba2037dbd8f48adc357d83",
version = "1.7.22",
sha256 = "5e3c8d0f965410ff12e90d6f8dc5df2fc09fd595a684d514616851ce7e94ae7d",
Copy link
Member

Choose a reason for hiding this comment

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

Any reason in particular we have to downgrade rules versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was mainly due to point 5 in the PR description. This was the highest version I could stay at with the least amount of changes. I will give another combination of dependencies a shot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to upgrade to a few different versions till v1.8.20 but all such changes open a can of worms that require several other dependencies to be updated

@KetanReddy
Copy link
Member

Definitely not something that needs to be in this PR, but for completeness across platforms, we should probably add issues to replicate the Badge component on all the other platforms

@A1shK
Copy link
Contributor Author

A1shK commented Jan 20, 2025

Definitely not something that needs to be in this PR, but for completeness across platforms, we should probably add issues to replicate the Badge component on all the other platforms

Makes sense. I will create those issues once this PR merges so that I can link the Android code

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

@KetanReddy KetanReddy KetanReddy left review comments

@sugarmanz sugarmanz Awaiting requested review from sugarmanz sugarmanz is a code owner

@brocollie08 brocollie08 Awaiting requested review from brocollie08 brocollie08 is a code owner

@adierkens adierkens Awaiting requested review from adierkens adierkens is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
minor Increment the minor version when merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@A1shK @nancywu1 @KetanReddy @brocollie08