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

Prevents the image size change from tracking each value continuously #17523

Merged
merged 1 commit into from
Nov 23, 2021

Conversation

emilylaguna
Copy link
Contributor

Project: #17503

Description

Prevents the image size change from tracking each value continuously

To test:

  1. Launch the app
  2. Tap My Site
  3. Tap your profile icon
  4. App App Settings
  5. Change the value of 'Max Image Upload Size' repeatedly without lifting up your finger
  6. Verify the value doesn't change
  7. Stop moving the slider
  8. Verify you see 🔵 Tracked: app_settings_image_optimization_changed <enabled: 1, value: VALUE>
  9. Verify the value is the value you stopped on IE: 1400x1400px should log value: 1400

Regression Notes

  1. Potential unintended areas of impact
    None, adding tracking.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manual testing

  3. What automated tests I added (or what prevented me from doing so)
    None.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

You can trigger an installable build for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

Copy link
Contributor

@momo-ozawa momo-ozawa left a comment

Choose a reason for hiding this comment

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

Works as described!

properties["enabled"] = (value != Int.max) as AnyObject
properties["value"] = value as Int as AnyObject
WPAnalytics.track(.appSettingsImageOptimizationChanged, withProperties: properties)
self.debounce(#selector(self.trackImageSizeChanged), afterDelay: 0.5)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍

Copy link
Contributor

@frosty frosty left a comment

Choose a reason for hiding this comment

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

This works mostly as expected, except for course because it's using a debounce it's possible we'll send multiple values if you pause for half a second while scrubbing.

An alternative would be to edit MediaSizeSliderCell and handle touchUpInside and touchUpOutside, and have an extra callback block to receive the final value when the touches end. We'd then use this final value for the tracking call.

Entirely up to you, depending on whether you think potentially logging some interim values is an issue or not! I'll approve as-is anyway 🙂

@emilylaguna
Copy link
Contributor Author

Good idea, I avoided doing that I wanted to reduce code modifications needed just for tracking. I don't want to accidentally break anything just by adding tracking. I think the number of interim values sent using the debounce will be pretty minimal for most users.

@emilylaguna emilylaguna merged commit 4f7f910 into develop Nov 23, 2021
@emilylaguna emilylaguna deleted the issue/17503-reduce-image-size-tracking branch November 23, 2021 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants