Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

uxcam integration and screen naming #474

Merged
merged 7 commits into from
May 12, 2022
Merged

uxcam integration and screen naming #474

merged 7 commits into from
May 12, 2022

Conversation

mijiga
Copy link
Contributor

@mijiga mijiga commented May 11, 2022

No description provided.

Copy link
Contributor

@androidmaven androidmaven left a comment

Choose a reason for hiding this comment

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

Code additions look excellent and straightforward, but just a minor however vital request.

Could we make the UXCam log consistently with other existing logs?

For example in all screens (fragments) we already have the AnalyticsUtil.logAnalyticsEvent(string, context) which logs for firebase, amplitude and appsflyer.

I think inside the functions would be the perfect place to add the UXCam.tagScreenName(); without the need for extra new strings.

However, this AnalyticsUtil.logAnalyticsEvent(string, context) is also used for other logs other than "visited screen" so we might want add an extra flag in the function for example AnalyticsUtil.logAnalyticsEvent(string, context, isForScreenVisit: Boolean? = false); and pass "true" inside each "visit logs"; this is so that we can appropriately check if we should log or not the UXCam in the AnalyticsUtil.logAnalyticsEvent.

Please let me know if you need further clarifications or its clear.
Thanks

@mijiga
Copy link
Contributor Author

mijiga commented May 12, 2022

@androidmaven makes sense, except when I do that the function calls without the isForScreenVisit variable set end up calling thelogAnalyticsEvent(event: String, context: Context, excludeAmplitude: Boolean) func. So maybe should just create another logAnalyticsEvent func with different variable order?

@androidmaven
Copy link
Contributor

@androidmaven makes sense, except when I do that the function calls without the isForScreenVisit variable set end up calling thelogAnalyticsEvent(event: String, context: Context, excludeAmplitude: Boolean) func. So maybe should just create another logAnalyticsEvent func with different variable order?

I see your point @mijiga . Instead of going through that change. You could do something like

logAnalyticsEvent(event: String, ...:Object){
val firstWord = event.split(" ").first()
if(firstWord == "visited") {
//then log UXCam
}
}

@mijiga
Copy link
Contributor Author

mijiga commented May 12, 2022

@androidmaven review

@androidmaven
Copy link
Contributor

@androidmaven review

Looking good Chief, but one last thing as a cleanup.
Can you run optimise code imports so the lingering import com.uxcam.UXCam in the fragments are being removed?

Thanks.

@mijiga
Copy link
Contributor Author

mijiga commented May 12, 2022

@androidmaven review

Looking good Chief, but one last thing as a cleanup. Can you run optimise code imports so the lingering import com.uxcam.UXCam in the fragments are being removed?

Thanks.

Done

@mijiga mijiga closed this May 12, 2022
@mijiga mijiga reopened this May 12, 2022
@androidmaven
Copy link
Contributor

Thanks! Currently doing functional testing. I will revert in a bit.

Copy link
Contributor

@androidmaven androidmaven left a comment

Choose a reason for hiding this comment

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

LGTM

@mijiga mijiga merged commit 5c995b3 into development May 12, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants