-
Notifications
You must be signed in to change notification settings - Fork 0
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
MOB-3869-Add a sample screen to example app of API customization #6
base: MOB-3712-example_app_for_meta_integration
Are you sure you want to change the base?
Conversation
Java/app/src/main/java/com/taboola/sdk4example/sdk_classic/MetaAdUICustomization.java
Show resolved
Hide resolved
@@ -115,6 +119,12 @@ public void onClick(View v) { | |||
case R.id.std_meta_classic_unit: | |||
fragmentToOpen = new MetaClassicUnitFragment(); | |||
break; | |||
case R.id.std_meta_ad_UI_customization: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielFrTB - What is the difference between these two screens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomer-br-taboola
std_meta_ad_UI_customization - regular Meta ad
std_meta_tbl_classic_unit_ad_UI_customization - meta_tbl_classic_unit (1x1 + feed)
} | ||
}); | ||
tblClassicUnit.setUnitExtraProperties(new HashMap<String, String>() {{ | ||
put(MetaConst.AUDIENCE_NETWORK_PLACEMENT_ID_KEY, MetaConst.AUDIENCE_NETWORK_CAROUSEL_PLACEMENT_ID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielFrTB - Is there a reason why you chose a carousel placement id here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomer-br-taboola
No, Changed to DEFAULT_LAYOUT_KEY
.setFontSize(TEXT_FONT_SIZE) | ||
.setFontDarkColor(Color.BLUE) | ||
.build(); | ||
TBLUiStyleProperties ctaStyleProperties = new TBLCallToActionButtonStylePropertiesBuilder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielFrTB - Since the default setting for the CTA button is to be visible, what is the point of showing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomer-br-taboola
I think that we need to display the all API we have.
I can remove if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielFrTB - If you want to display all the APIs we have, choose an option that shows to publishers how it works. This way, publishers might think they need to do this (even though they don't since the default is to show the CTA button).
tblMetaClassicUnit.setUnitExtraProperties(new HashMap<String, String>() {{ | ||
put(MetaConst.AUDIENCE_NETWORK_PLACEMENT_ID_KEY, MetaConst.AUDIENCE_NETWORK_PLACEMENT_ID); | ||
}}); | ||
TBLUiStyleProperties brandingStyleProperties = new TBLTextStylePropertiesBuilder(ELEMENT_TYPE_BRANDING) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielFrTB - Comments from the previous screen also apply here.
@@ -12,4 +12,10 @@ public class MetaConst { | |||
public static final String DEFAULT_LAYOUT_KEY = "default"; | |||
public static final String TEST_LAYOUT_IMAGE_LINK_TYPE = "image_link"; | |||
public static final String TEST_LAYOUT_CAROUSEL_TYPE = "carousel"; | |||
// UI customization | |||
public static final String ELEMENT_TYPE_BRANDING = "branding"; | |||
public static final String DARK_NODE = "darkMode"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielFrTB - Why is the dark mode key part of the UI Customizations here and not in the Kotlin part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomer-br-taboola aligned
Log.d(MetaAdUICustomizationsFragment.TAG, "onAdReceiveFail $error") | ||
} | ||
}) | ||
tblClassicUnit.setAdTypeForDebug(MetaConst.TEST_LAYOUT_IMAGE_LINK_TYPE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielFrTB - I think that in these screens (as well as the Java counterparts), comments can help publishers understand what is going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please go over the comments.
// UI customization | ||
public static final String ELEMENT_TYPE_BRANDING = "branding"; | ||
public static final String DARK_NODE = "darkMode"; | ||
public static final String FONT_TYPEFACE_ARIAL_BOLD = "arial_bold"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielFrTB - No need to write typeface and font. Choose one and stick to it.
public static final String FONT_TYPEFACE_ARIAL_BOLD = "arial_bold"; | ||
public static final int TEXT_FONT_SIZE = 20; | ||
public static final int NUMBER_OF_LINES = 2; | ||
public static final float AMOUNT_OF_LINES_BETWEEN_LINES = 0.5f; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielFrTB - I think the name of this constant is wrong, AMOUNT_OF_LINES_BETWEEN_LINES
. I believe it should be amount of space between lines.
https://brain.taboola.com/display/MOBILE/TBLUiStyleProperties
|
||
import androidx.fragment.app.FragmentActivity; | ||
|
||
public class MetaUtils { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielFrTB - There is no logic that is specific to Meta here. Consider renaming. Maybe there is another already present utility class that you can add this method to.
// Create custom style properties for the call to action button | ||
TBLUiStyleProperties ctaStyleProperties = new TBLCallToActionButtonStylePropertiesBuilder() | ||
// Set the visibility of the call to action, the CTA button will be displayed by default if | ||
// you want to hide it you need to pass true to the setVisibility method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielFrTB - Please re-read the comment here. It is misleading.
Kotlin/app/src/main/java/com/taboola/kotlin/examples/screens/utils/MetaUtils.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielFrTB - Please go over all comments.
.setTypeface(font) | ||
.build(); | ||
|
||
// Create custom style properties for the call to action button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielFrTB - Why is this code commented out? As mentioned in my previous comment, show the option to publisher that is NOT the default (meaning, hide the CTA).
Leaving commented out code is not a good practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomer-br-taboola
Mosh ask to put here a commented code for demonstration
.build(); | ||
|
||
// Create custom style properties for the call to action button | ||
TBLUiStyleProperties ctaStyleProperties = new TBLCallToActionButtonStylePropertiesBuilder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielFrTB - Here you didn't apply any changes.
.setTypeface(typefaceArielBold) | ||
.build() | ||
// Create custom style properties for the call to action button | ||
val callToActionButtonStyleProperties = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielFrTB - Here you didn't apply any changes.
.build() | ||
|
||
// // Create custom style properties for the call to action button | ||
// val callToActionButtonStyleProperties = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielFrTB - Why is this code commented out? As mentioned in my previous comment, show the option to publisher that is NOT the default (meaning, hide the CTA).
Leaving commented out code is not a good practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomer-br-taboola
Mosh ask to put here a commented code for demonstration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielFrTB - Please go over comments
Java:
Kotlin: