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

Deprecate analytics function setCurrentScreen #6269

Merged
merged 3 commits into from
May 23, 2022

Conversation

dwyfrequency
Copy link
Contributor

Mark setCurrentScreen function within analytics as deprecated. Users should utilize logEvent with eventName as 'screen_view' and add the relevant eventParams. For more information, see https://firebase.google.com/docs/analytics/screenviews#web-version-9

@changeset-bot
Copy link

changeset-bot bot commented May 11, 2022

⚠️ No Changeset found

Latest commit: f561e54

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 11, 2022

@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 11, 2022

@dwyfrequency dwyfrequency marked this pull request as ready for review May 11, 2022 23:13
@@ -53,6 +53,8 @@ export async function logEvent(
/**
* Set screen_name parameter for this Google Analytics ID.
*
* @deprecated Use logEvent with eventName as 'screen_view' and add relevant eventParams.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add backticks to function and variable names (logEvent, eventName, eventParams)? I think screen_view makes sense with single quotes since it's a string. Actually I think you can link directly to them if they're documented with {@link logEvent}, see https://github.com/firebase/firebase-js-sdk/blob/master/packages/analytics/src/api.ts#L85 You can generate the docs and double check the links work.

I think only logEvent and eventParams warrant a link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the {@links logEvent} and added backticks where necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, slipped my mind we can actually link guide docs. Can you add a second line

 * See {@link https://firebase.google.com/docs/analytics/screenviews
 * | Track Screenviews}.

That's from the doc comment for the logEvent overload for 'screen_view' but since we can't link to that overload might as well put it right here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the documentation link for Track Screenviews

@@ -5215,6 +5215,8 @@ declare namespace firebase.analytics {

/**
* Use gtag 'config' command to set 'screen_name'.
*
* @deprecated Use logEvent with eventName as 'screen_view' and add relevant eventParams.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above. Unfortunately links work a little differently in this file as you have to specify the namespace and you're allowed to customize the link text I guess. Search for "@link" in this file for examples, one example is {@link firebase.analytics.Analytics Analytics} Again, generate docs and make sure the link looks right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the {@links logEvent} and added backticks where necessary. The logEvent link here auto resolves to the compat version firebase.analytics.Analytics.logEvent in vscode and in the resulting html files post docgen:compat

@hsubox76 hsubox76 requested a review from egilmorez May 16, 2022 18:27
@egilmorez egilmorez requested a review from markarndt May 16, 2022 18:33
Copy link
Collaborator

@markarndt markarndt left a comment

Choose a reason for hiding this comment

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

LGTM

@dwyfrequency dwyfrequency merged commit 11d37ed into master May 23, 2022
@dwyfrequency dwyfrequency deleted the deprecateSetCurrentScreen branch May 23, 2022 19:00
@firebase firebase locked and limited conversation to collaborators Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants