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

Add default configuration for analytic vendors. #7034

Merged
merged 7 commits into from Apr 27, 2022

Conversation

dhaval-parekh
Copy link
Collaborator

@dhaval-parekh dhaval-parekh commented Apr 7, 2022

Summary

Fixes #7019

Pre-fill default config when the user selects analytic vendor on the AMP setting page.

analytic-default-config.mov

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@dhaval-parekh dhaval-parekh self-assigned this Apr 7, 2022
@dhaval-parekh dhaval-parekh force-pushed the enhancement/7019-pre-fill-analytic-vendors branch 2 times, most recently from 5e5f3bd to 1a5500c Compare April 11, 2022 08:50
@westonruter westonruter added this to the v2.3 milestone Apr 14, 2022
@@ -18,59 +18,10 @@ import { Button, PanelRow, BaseControl, VisuallyHidden } from '@wordpress/compon
*/
import { Options } from '../components/options-context-provider';
import { AMPNotice, NOTICE_SIZE_SMALL } from '../components/amp-notice';
import vendorConfigs from './vendor-configs';

const GOOGLE_ANALYTICS_VENDOR = 'googleanalytics';
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we still need this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, This variable is used below for message.

@milindmore22
Copy link
Collaborator

@dhaval-parekh should we add a notice with (link) for 10 vendors something like 🔗 "Learn More" pointing to the respective vendor's documentation link

Example:
image

@dhaval-parekh dhaval-parekh marked this pull request as ready for review April 20, 2022 08:53
@dhaval-parekh
Copy link
Collaborator Author

should we add a notice with (link) for 10 vendors something like 🔗 "Learn More" pointing to the respective vendor's documentation link

Yes, We can add it.

Let me check if i can find the documentation for those vendors.

@dhaval-parekh dhaval-parekh force-pushed the enhancement/7019-pre-fill-analytic-vendors branch from 1a5500c to 1f2a039 Compare April 20, 2022 11:17
@github-actions
Copy link
Contributor

github-actions bot commented Apr 20, 2022

Plugin builds for 16d597c are ready 🛎️!

@dhaval-parekh dhaval-parekh force-pushed the enhancement/7019-pre-fill-analytic-vendors branch from 1f2a039 to 21f9ddb Compare April 21, 2022 08:14
Copy link
Collaborator

@milindmore22 milindmore22 left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

@westonruter
Copy link
Member

There's an E2E test failing, but I'm updating with develop to see if it is a temporary issue.

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

The E2E tests need to be fixed.

@westonruter westonruter merged commit 3b21e64 into develop Apr 27, 2022
@westonruter westonruter deleted the enhancement/7019-pre-fill-analytic-vendors branch April 27, 2022 17:29
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pre-fill default configuration for top analytic vendors.
3 participants