Skip to content

Conversation

@navratan-soni
Copy link
Contributor

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov
Copy link

codecov bot commented Aug 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.08%. Comparing base (0f1e0e9) to head (32feb97).
Report is 8 commits behind head on feature/content-card.

Additional details and impacted files
@@                   Coverage Diff                   @@
##             feature/content-card      #70   +/-   ##
=======================================================
  Coverage                   92.08%   92.08%           
  Complexity                    289      289           
=======================================================
  Files                          38       38           
  Lines                        1871     1871           
  Branches                      174      174           
=======================================================
  Hits                         1723     1723           
  Misses                         86       86           
  Partials                       62       62           
Flag Coverage Δ
unit-tests 92.08% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

}


class SmallImageAepUi(
Copy link
Contributor

@spoorthipujariadobe spoorthipujariadobe Sep 27, 2024

Choose a reason for hiding this comment

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

Can we move this to its own file?

Can we decide if it needs to be Aep or AEP and apply it across all file and class names? I vote for the latter since its an acronym and it would be consistent with the class names in Core UI service

Can we also change ui to UI?

/**
* Composable for rendering a list of AEP UI components.
* Maintains a list of AEP UI components and renders them using the provided container.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add descriptions of what the @params mean here

uiAsComposable.invoke()
}

Button(onClick = { viewModel.loadMore() }, Modifier.fillMaxWidth()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This button was added for testing. We don't need it in the actual implementation

}
}

fun loadMore() {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we call this refreshList? The content provider is expected to update the contentFlow when this is called thereby refreshing the list view

Copy link
Contributor

Choose a reason for hiding this comment

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

Also we don't need to support this right now. It was just there for POC. We can add it later if needed


/**
* Composable for rendering a Small Image Card
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add description for @params
Also let's make this public. Apps should be able to use this composable, just like the list one

sealed interface AepUiState

data class SmallImageUIState(val title: String?, val dismissed: Boolean = false) : AepUiState
data class LargeImageUIState(val dismissed: Boolean = false) : AepUiState
Copy link
Contributor

Choose a reason for hiding this comment

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

LargeImageUIState is out of the scope of this release let's remove it


sealed interface AepUiState

data class SmallImageUIState(val title: String?, val dismissed: Boolean = false) : AepUiState
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this to its own file
It will need to be changed in future PRs to reflect actual values the state needs to hold

import androidx.compose.ui.text.style.TextAlign

// only needed when we support server side styling
fun AEPText.getComposeTextStyle(): TextStyle {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this class? There's no support for server side styling in AJO. We can add it when we need to

val smallImageAepUiStyle: SmallImageAepUiStyle = SmallImageAepUiStyle()
)

class SmallImageAepUiStyle(
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this also to its own file

*/
suspend fun getContent(): Flow<List<AepUiTemplate>>

suspend fun refreshContent()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a description for this method

/**
     * Refreshes the content for the UI by updating the flow provided in getContent`
*/

import com.adobe.marketing.mobile.aepcomposeui.aepui.AepUI
import com.adobe.marketing.mobile.aepcomposeui.aepui.state.AepUiState

sealed interface UIEvent<T : AepUiTemplate, S : AepUiState> {
Copy link
Contributor

@spoorthipujariadobe spoorthipujariadobe Sep 27, 2024

Choose a reason for hiding this comment

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

Let's add some description for this interface and what T and S mean


import com.adobe.marketing.mobile.aepcomposeui.interactions.UIEvent

interface AepUiEventObserver {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here. Let's add a description for the interface and what onEvent is for


import com.adobe.marketing.mobile.aepuitemplates.utils.AepUiTemplateType

class SmallImageTemplate() : AepUiTemplate {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a public class. Please provide some description here

enum class AepUiTemplateType(val typeName: String) {

SMALL_IMAGE("SmallImage"),
LARGE_IMAGE("LargeImage")
Copy link
Contributor

Choose a reason for hiding this comment

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

We're not adding large image in the first iteration. Can we remove this enum?

notificationbuilderMavenRepoDescription=Android Notification Builder library for Adobe Mobile Marketing

aepComposeUiModuleName = aepcomposeui
aepComposeUiVersion=1.0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be 3.0.0. We align major versions of all SDKs

aepComposeUiMavenRepoDescription = Android AEP Compose UI library for Adobe Mobile Marketing

aepUiTemplatesModuleName = aepuitemplates
aepUiTemplatesVersion=1.0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here. Let's make it 3.0.0

Copy link
Contributor

@spoorthipujariadobe spoorthipujariadobe left a comment

Choose a reason for hiding this comment

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

How about we have a folder structure like this inside aepui

composable
           interfaces
           <implementation of the interface for a specific type>

Same for aepUi, style and state

}

dependencies {
implementation("com.adobe.marketing.mobile:core:$mavenCoreVersion")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this library need MobileCore dependency?

val aepUiTemplatesMavenRepoDescription: String by project

aepLibrary {
namespace = "com.adobe.marketing.mobile.aepcomposeui"
Copy link
Contributor

Choose a reason for hiding this comment

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

namespace needs to be com.adobe.marketing.mobile.aepuitemplates

}

dependencies {
implementation("com.adobe.marketing.mobile:core:$mavenCoreVersion")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need core dependency here?

Comment on lines +36 to +40
[plugins]
android-application = { id = "com.android.application", version.ref = "agp" }
jetbrains-kotlin-android = { id = "org.jetbrains.kotlin.android", version.ref = "kotlin" }
android-library = { id = "com.android.library", version.ref = "agp" }

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +17 to +21
androidx-core-ktx = { group = "androidx.core", name = "core-ktx", version.ref = "coreKtx" }
junit = { group = "junit", name = "junit", version.ref = "junit" }
androidx-junit = { group = "androidx.test.ext", name = "junit", version.ref = "junitVersion" }
androidx-espresso-core = { group = "androidx.test.espresso", name = "espresso-core", version.ref = "espressoCore" }
androidx-lifecycle-runtime-ktx = { group = "androidx.lifecycle", name = "lifecycle-runtime-ktx", version.ref = "lifecycleRuntimeKtx" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here. All of them are included in aep-plugin

androidx-lifecycle-runtime-compose = { group = "androidx.lifecycle", name = "lifecycle-runtime-compose", version.ref = "lifecycleRuntimeCompose" }
androidx-lifecycle-viewmodel-compose = {group = "androidx.lifecycle", name = "lifecycle-viewmodel-compose", version.ref = "lifecycleViewModelCompose"}
androidx-activity-compose = { group = "androidx.activity", name = "activity-compose", version.ref = "activityCompose" }
androidx-compose-bom = { group = "androidx.compose", name = "compose-bom", version.ref = "composeBom" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to use compose bom? aep-plugin has almost all the dependencies you need. can we include the ones not present there directly using the same version through the BuildConstants?
Example from core https://github.com/adobe/aepsdk-core-android/blob/main/code/core/build.gradle.kts#L27-L32

@@ -0,0 +1,40 @@
[versions]
Copy link
Contributor

Choose a reason for hiding this comment

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

can we verify if we need this file? most of them are included in aep-plugin

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.

2 participants