Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[firebase_analytics] add missing named events tracking #1850

Merged
merged 6 commits into from Jul 15, 2019
Merged

[firebase_analytics] add missing named events tracking #1850

merged 6 commits into from Jul 15, 2019

Conversation

juliocbcotta
Copy link
Contributor

Description

This PR:

  • add some named events for firebase analytics.
  • updated logShare to require a third parameter.
  • update the list of reserved event names.
  • include some constants that are in the documentation, but are not used. (yet?)

NOTE: Should a new parameter in the logShare force major version update?

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

Copy link
Contributor

@collinjackson collinjackson left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good. It's a bit unfortunate to do a major version bump for such a small change but I think we should probably stick to semVer here.

@collinjackson collinjackson merged commit 91f971a into flutter:master Jul 15, 2019
@juliocbcotta juliocbcotta deleted the plugin/firebase_analytics branch July 17, 2019 03:38
mithun-mondal pushed a commit to bKash-developer/plugins that referenced this pull request Aug 6, 2019
@dotdoom
Copy link
Contributor

dotdoom commented Oct 20, 2019

Please forgive me my ignorance here, but what's the point of marking the new parameter @required and making this a breaking change for all existing users of firebase_analytics?

The Firebase documentation does not talk about required parameters, but I'm interested in learning what drives such strict limitations in Flutter.

@juliocbcotta
Copy link
Contributor Author

juliocbcotta commented Oct 22, 2019

@dotdoom , there is no indication of that parameter as being optional in the documentation neither.

@dotdoom
Copy link
Contributor

dotdoom commented Oct 22, 2019

The top of the documentation page I mentioned above says that all events (except some google_ presets) are custom and so are parameters. It gives a list of events as a suggestion.

But I am buying ambiguous documentation as a reason, although it's unfortunate that we get to break users every time those suggestions change.

julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
4 participants