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

Analytics #4559

Merged
merged 51 commits into from
Dec 14, 2021
Merged

Analytics #4559

merged 51 commits into from
Dec 14, 2021

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented Nov 24, 2021

Content

Analytics framework is set up.
No event is tracked yet, but could by nice to handle that in another PR to let other developer add their analytics Events.

Analytics Optin screen will be displayed for every users when the home screen is displayed for the first time (so either after the first sigin/signup, or after an upgrade of the application).

Opt in screen

Dark Light
image image

New section in settings

image

This PR also contain a debug screen, only accessible on debug build:

image

TODO

  • Iterate on the design for opt in (waiting for @amshakal)
  • Update the policy link (waiting for @deniseal)
  • Show the opt in screen from the setting, rather than just having a switch (I am on it).

@github-actions
Copy link

github-actions bot commented Nov 24, 2021

Unit Test Results

  66 files  ±0    66 suites  ±0   57s ⏱️ -1s
135 tests ±0  135 ✔️ ±0  0 💤 ±0  0 ±0 
418 runs  ±0  418 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit be33a53. ± Comparison against base commit 5ea7f3c.

♻️ This comment has been updated with latest results.

- name: Run analytics import script
run: ./tools/import_analytic_plan.sh
- name: Create Pull Request for analytics plan
uses: peter-evans/create-pull-request@v3
Copy link
Contributor

Choose a reason for hiding this comment

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

another possibility is to .jar the generated output as part of the schema repository and upload to github packages, that way dependabot can handle updates and we can ensure the generated artifact compiles in isolation

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, @pixlwave told me about that possibility. It's probably a cleaner solution, but I have never done that before 🙈

/**
* Read the analytics config from the Build config
*/
fun getConfig(): AnalyticsConfig? {
Copy link
Contributor

Choose a reason for hiding this comment

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

not for this PR, eventually we might want to wrap and inject the BuildConfig rather than directly accessing it (will enable the config to be shared across modules)

Copy link
Member Author

Choose a reason for hiding this comment

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

I have updated the way developer can configure the analytics in c1438f0

Copy link
Contributor

@ouchadam ouchadam left a comment

Choose a reason for hiding this comment

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

from what I can tell, this looks good to me (assuming the tracking is coming through!)

@bmarty bmarty force-pushed the feature/bma/posthog branch 2 times, most recently from f4578cc to 97e2456 Compare December 1, 2021 08:39
@bmarty bmarty marked this pull request as ready for review December 7, 2021 17:04
@bmarty
Copy link
Member Author

bmarty commented Dec 8, 2021

Updated opt-in screen layout

image

@bmarty
Copy link
Member Author

bmarty commented Dec 9, 2021

@amshakal

7'' tablets 10'' tablets
image image

We can iterate on the max width for CTA buttons and text block in a dedicated PR, since it will impact other screens of the app.

@amshakal
Copy link

amshakal commented Dec 9, 2021

Thanks for sharing these. This is looking good, I just have two comments:

  1. For larger screens, can we increase the padding on left and right of the text, since we have all this vertical space. This will make it easier to read.
  2. For the button, would it be more helpful to have % of screen it should cover or pixels? I think capping to 600px for portrait and 800px for landscape mode might work. This works out to be around 60% i.e. the button width is 60% of the screen width. There doesn't seem to be a set guidelines for buttons on tablet.

@bmarty
Copy link
Member Author

bmarty commented Dec 13, 2021

@amshakal I have added code to limit the width of the content on tablet. We can iterate later for a proper way to support tablet across the whole code base.

@bmarty bmarty enabled auto-merge December 13, 2021 22:33
@bmarty bmarty merged commit f4cfb5d into develop Dec 14, 2021
@bmarty bmarty deleted the feature/bma/posthog branch December 14, 2021 08:31
@amshakal
Copy link

Thanks Benoit! I had a discussion with the design team about tablet designs and the plan is to take it up in the new year and have it be included in the design system.

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.

3 participants