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

feat: Pages with steady decline of conversion #293

Draft
wants to merge 44 commits into
base: main
Choose a base branch
from
Draft

Conversation

Dereje24
Copy link

Please ensure your pull request adheres to the following guidelines:

  • make sure to link the related issues in this description
  • when merging / squashing, make sure the fixed issue references are visible in the commits, for easy compilation of release notes

Related Issues

Thanks for contributing!

@Dereje24 Dereje24 changed the title SITES-22537 feat: Pages with steady decline of conversion feat: Pages with steady decline of conversion Jul 17, 2024
@Dereje24 Dereje24 requested a review from ekremney July 17, 2024 01:09
Copy link

This PR will trigger a minor release when merged.

Copy link
Member

@ekremney ekremney left a comment

Choose a reason for hiding this comment

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

we dont have a way to calculate conversions accurately. Maybe we should change Pages with steady decline of conversion rate to Pages with steady decline of click-through-rate. What do you think @ramboz ?

@Dereje24 Dereje24 requested review from ramboz and ravkiran July 18, 2024 18:07
Copy link

Choose a reason for hiding this comment

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

I'd rename the file to reflect the exact use case you handle. we'll likely have many opportunity implementations down the road

Comment on lines 75 to 76
// Initialize a Set to hold the unique selectors for this bundle
let uniqueSelectors = new Set();
const uniqueSelectors = new Set();
Copy link

Choose a reason for hiding this comment

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

I'd move the declaration outside of the loop so you re-use the same variable and avoid allocation of hundreds of different ones.

    let uniqueSelectors;
    for (const bundle of bundles) {
      // Initialize a Set to hold the unique selectors for this bundle
      uniqueSelectors = new Set();

let uniqueSelectors = new Set();
const uniqueSelectors = new Set();

let totalClicks = 0;
Copy link

Choose a reason for hiding this comment

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

Same here, I'd move the declaration outside of the loop

@@ -83,13 +91,13 @@ function handler(bundles) {

// Iterate over the unique selectors and increment their count in the global selectors object
for (const source of uniqueSelectors) {
globalSelectors[source] = (globalSelectors[source] || 0) + 1;
const count = uniqueSelectors[source] || 0;
Copy link

Choose a reason for hiding this comment

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

Again here as well

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