Skip to content

Remove requirement for analytics to entries to have type supplied#4802

Closed
westonruter wants to merge 6 commits intodevelopfrom
update/analytics-type-requirement
Closed

Remove requirement for analytics to entries to have type supplied#4802
westonruter wants to merge 6 commits intodevelopfrom
update/analytics-type-requirement

Conversation

@westonruter
Copy link
Copy Markdown
Member

@westonruter westonruter commented May 30, 2020

Summary

This addresses an issue raised in support topic where in-house analytics (which are supposed to lack a type attribute) are

  • Remove required attribute from Type input field and server-side validation.
  • Omit type from amp-analytics tag when Type is empty.
  • Remove the unnecessary readonly ID input field from the form entries.
  • Use UUID to generate unique IDs rather than hashing type and contents.
  • Stop needlessly showing truncating ID in entry heading on admin screen, and use ordered list instead to differentiate entries.

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • 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).

@googlebot googlebot added the cla: yes Signed the Google CLA label May 30, 2020
@westonruter westonruter force-pushed the update/analytics-type-requirement branch from 9908861 to 7198677 Compare May 30, 2020 19:49
@westonruter westonruter marked this pull request as ready for review May 30, 2020 19:52
@westonruter westonruter added this to the v1.5.4 milestone May 30, 2020
@westonruter westonruter modified the milestones: v1.5.4, v1.6 Jun 16, 2020
@westonruter westonruter requested a review from pierlon June 16, 2020 03:10
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 16, 2020

Plugin builds for ba895cc are ready 🛎️!

@westonruter westonruter modified the milestones: v1.5.4, v1.6 Jun 16, 2020
@westonruter westonruter removed this from the v1.6 milestone Jun 16, 2020
@westonruter westonruter removed the request for review from pierlon June 16, 2020 03:35
@westonruter
Copy link
Copy Markdown
Member Author

westonruter commented Jun 16, 2020

Rather than being satisfied with this as-is, in 1.6+ I'd rather see this Analytics screen removed entirely and moved into an “Advanced” area of the main settings screen. This section would be collapsed by default. We'd use the REST API to update the Analytics and get rid of the current codebase entirely. See #2317.

@westonruter westonruter added this to the v1.7 milestone Jun 16, 2020
@pierlon
Copy link
Copy Markdown
Contributor

pierlon commented Aug 30, 2020

Removing this from the v2.1 milestone since it's already been fixed in #5155.

@pierlon pierlon deleted the update/analytics-type-requirement branch August 30, 2020 05:57
@pierlon pierlon removed this from the v2.1 milestone Aug 30, 2020
@westonruter westonruter added this to the v2.0 milestone Aug 30, 2020
@westonruter
Copy link
Copy Markdown
Member Author

westonruter commented Aug 30, 2020

Moved to v2.0

@westonruter
Copy link
Copy Markdown
Member Author

Oh, woops. This PR was not merged. It is obsolete.

@westonruter westonruter removed this from the v2.0 milestone Aug 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Signed the Google CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants