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

Handle missing scroll depth #2 #5017

Merged
merged 23 commits into from
Jan 29, 2025
Merged

Handle missing scroll depth #2 #5017

merged 23 commits into from
Jan 29, 2025

Conversation

macobo
Copy link
Contributor

@macobo macobo commented Jan 27, 2025

Changes

Promised refactor of #4979

This PR:

  1. Adds a new scroll_depth_visible_at field to Team
  2. Extracts scroll_depth module for checking feature visibility/availability.
  3. Makes the code less dependant on clickhouse being available
  4. Add an extra check next to the feature flag - if site.engagement_metrics_enabled_at is set, show scroll_depth on the dashboard + include it in dashboard CSV and Full CSV exports

To be followed up with:

  1. Instead of checking clickhouse on page load, use a cache worker for this. This is a blocker for release of the feature.

RobertJoonas and others added 21 commits January 14, 2025 09:25
@RobertJoonas
add migration
c3c1039
@RobertJoonas
move scroll_depth_enabled? fn
7974895
@RobertJoonas
maybe set engagement_metrics_enabled_at when requesting dashboard
4d02ae4
@RobertJoonas
maybe set engagement_metrics_enabled_at in shared_link action
8ab805b
@RobertJoonas
maybe set engagement_metrics_enabled_at on full export
9850450
@RobertJoonas
fix tests
fb279b3
@RobertJoonas
feature gate scroll depth on the dashboard with site.engagement_metri…
cdf2c83 
…cs_enabled_at
@RobertJoonas
feature gate scroll depth in full export too
19fdda7
@RobertJoonas
fix npm ci
8c3c922
@macobo
Rename things into FE, remove unneccessary flag checks
9631f60
@macobo
Continue with renaming
cc26755
@macobo
Merge remote-tracking branch 'origin/master' into handle-missing-scro…
8ea7088 
…ll-depth
@macobo
Rename site flag to be more descriptive
8ea9eca
@macobo
Move business logic, calculate based on scroll depth, make more preca…
80fc6c6 
…lculatable
@macobo
Some docs
cf2bb17
@macobo
Rename to site.scrollDepthVisible in frontend
7c1bf77
@macobo
Update migration
422ed9a
@macobo
Fix template
957f029
@macobo
Remove boilerplate from tests
3a94d2c
@macobo
Update tests
a6516cd
@macobo
More straight-forward test
8586103
@macobo macobo mentioned this pull request Jan 27, 2025
Merged
@macobo macobo marked this pull request as ready for review January 27, 2025 07:26
RobertJoonas
RobertJoonas approved these changes Jan 27, 2025
View reviewed changes
Copy link
Contributor

@RobertJoonas RobertJoonas left a comment

Choose a reason for hiding this comment

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

Looks good to me - just one question inline.

priv/repo/migrations/20250109114738_add_scroll_depth_visible_at_to_site.exs Outdated Show resolved Hide resolved
@macobo macobo added this pull request to the merge queue Jan 29, 2025
Merged via the queue into master with commit 7c6ba04 Jan 29, 2025
8 checks passed
@macobo macobo deleted the missing-scroll-depth branch January 29, 2025 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RobertJoonas RobertJoonas RobertJoonas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@macobo @RobertJoonas