feature(addon-google-analytics) utilise storybook addonParameter instead of window/global scope. #11336

alexanderjeurissen · 2020-06-27T12:37:08Z

What I did

I changed the google analytics add-on to utilise addonParameters instead of globals.
This has several benefits:

It makes the google analytics add-on more in line with other storybook add-on configurations.
It opens up new ways to extend the configuration in future PR's, such as:
- specifying GA option overrides for specific stories
- allowing opt-in / opt-out of certain GA tracking events
- Expanding the number of events that we allow to track such as: (search, keydown, control interaction etc.)

How to test

Is this testable with Jest or Chromatic screenshots?
- This is probably testable with Jest, although I could not find existing test coverage for this.
Does this need a new example in the kitchen sink apps?
- I don't think so, as we currently don't have analytics in the kitchen sink apps.
Does this need an update to the documentation?
- Yes, I updated the README to reflect the configuration change

shilman · 2020-06-27T13:07:09Z

@alexanderjeurissen thanks for the PR -- def a big improvement over the current version!

@ndelangen does this belong in addons.setConfig?

alexanderjeurissen · 2020-06-27T13:15:37Z

@shilman you're welcome, I'm currently working around some of these limitations more specifically the extensibility of the current google analytics package in our storybook at HackerOne.

This is a first step to get there, after this gets merged I want to explore expanding the CORE events that we track. I envision that users should be able to specify what Storybook events they want to track, And maybe allow for a callback so that they can further customise the behaviour.

I also want to explore putting React GA in a provider so that we can also consume ReactGA in stories directly without having to re-initialize.

I recently authored http://npmjs.com/@alexanderjeurissen/use-reactga which could help with this specifically.

alexanderjeurissen · 2020-06-27T13:22:10Z

@shilman @ndelangen I'm not sure if this belongs in addons.setConfig for now it might make sense given that the configuration is global to Storybook. But if at a future point we want to enable users to override these settings per story, addons.setConfig might not make sense anymore ? unless there is a way to override setConfig per story.

shilman · 2020-06-27T13:32:49Z

@alexanderjeurissen i agree that parameters make more sense if we want to vary these things per story. is that something you have planned?

one other thing. this is a breaking change, so we should make sure to get it in before 6.0 goes out if we're going to do it. if you're planning on making more breaking changes in the near future, please consider that as well!

alexanderjeurissen · 2020-06-27T13:43:00Z

@alexanderjeurissen i agree that parameters make more sense if we want to vary these things per story. is that something you have planned?

Yes I think it makes sense to allow for this given the minimal overhead that this introduces. I think that extending the current "global" parameters to allow for story specific ones is a non-breaking change as it only requires re-initializing a central ReactGA the API remains the same for global parameters.

one other thing. this is a breaking change, so we should make sure to get it in before 6.0 goes out if we're going to do it. if you're planning on making more breaking changes in the near future, please consider that as well!

That makes sense, do you have an estimate as to when you expect a feature freeze for the 6.0 release to take effect ? That way I can plan some of these changes accordingly.

alexanderjeurissen · 2020-06-27T13:47:18Z

lib/cli/versions.json

+  "@storybook/angular": "6.0.0-beta.37",
+  "@storybook/aurelia": "6.0.0-beta.37",
+  "@storybook/ember": "6.0.0-beta.37",
+  "@storybook/html": "6.0.0-beta.37",
+  "@storybook/marionette": "6.0.0-beta.37",
+  "@storybook/marko": "6.0.0-beta.37",
+  "@storybook/mithril": "6.0.0-beta.37",
+  "@storybook/preact": "6.0.0-beta.37",
+  "@storybook/rax": "6.0.0-beta.37",
+  "@storybook/react": "6.0.0-beta.37",
+  "@storybook/riot": "6.0.0-beta.37",
+  "@storybook/server": "6.0.0-beta.37",
+  "@storybook/svelte": "6.0.0-beta.37",
+  "@storybook/vue": "6.0.0-beta.37",
+  "@storybook/web-components": "6.0.0-beta.37"


Not sure where these changes are coming from 🤔

alexanderjeurissen · 2020-06-27T14:07:46Z

@shilman is there anything I need to do for the Danger JS failure ?

shilman · 2020-06-27T14:34:56Z

@alexanderjeurissen We’re scheduled to do the first RC this week. I think we can fudge it a bit for analytics because it’s not as widely used as some addons. We can also add this as a feature in 6.1 as needed as long as we provide back compat, which wouldn’t be hard in this case

shilman · 2020-06-27T14:35:33Z

Re: danger, don’t worry about it. It always fails on PRs from forks

alexanderjeurissen · 2020-06-27T21:52:54Z

@shilman I made some changes, The default assignment in the addonParameter destructuring should result in backwards compatibility.

Can you sanity check and if backwards compatibility looks good remove the BREAKING CHANGE label accordingly ?

alexanderjeurissen · 2020-06-27T22:25:45Z

I also created a followup PR that is based on this one (#11340) Let me know how you want to move forward with merging / releasing.

alexanderjeurissen · 2020-06-30T21:26:45Z

@shilman I respect that you and the other maintainers are currently very busy getting Storybook 6.0 ready to ship.

I wanted to inquire if there is any remaining action required on my end, or if this PR is ready to review as is.

shilman · 2020-06-30T23:44:22Z

@alexanderjeurissen Looking good AFAIK. I’ll try to review today

ndelangen · 2020-07-03T08:52:33Z

addons/google-analytics/README.md

+addParameters({
+  analytics: {
+    reactGAId: 'UA-000000-01',
+    reactGAOptions: {},
+  },
+});


setConfig would make more sense.

Even more would defining this in main.js, and have a preset push it into the proper place where the runtime expects it. (such as setConfig)

@ndelangen can you elaborate why setConfig would be better in this scenario ?

If I understand correctly, setConfig does not allow for local story overrides. I can think of future enhancements where we would want to allow users to specify different reactGA options or tracking codes for certain stories. Would that still work with a setConfig approach ?

allow users to specify different reactGA options or tracking codes for certain stories

I suspect there's no need for that.. Do you?

I do, having different tracking codes is likely desired in cases where a medium to big UI library is published to different scoped NPM packages but hosted in a single repository.

for instance say we have a UI kit that is split up across the following NPM packages:

@same_fancy_library/components

@same_fancy_library/layout

@some_fancy_library/utils

This pattern of co-located but separately published packages is getting more popular/common. In those cases one could make the case for separate storybook instances, however I think that having a single storybook instance that hosts all stories is preferable from a end user UX perspective.

Furthermore, I've seen instances where submodules of big component libraries are maintained by different teams who each have their own GA View / property that they want to observe.

I think that the overhead of enabling this use-case is so small that it is likely worth exploring.

@ndelangen WDYT ?

I gave this some more thought, and given that storybook will get composition soon (#9210) the use-case I described is likely better served by using composition and defining a per "nested" storybook configuration.

I'll look into updating this PR to use setConfig

stale · 2020-08-16T07:37:51Z

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

stale · 2020-09-07T12:59:14Z

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

stale · 2020-10-12T00:03:16Z

Hey there, it's me again! I am going close this issue to help our maintainers focus on the current development roadmap instead. If the issue mentioned is still a concern, please open a new ticket and mention this old one. Cheers and thanks for using Storybook!

Let Storybook google analytics addon use parameters instead of globals.

351b947

alexanderjeurissen changed the title ~~WIP: Let Storybook google analytics addon use parameters instead of globals.~~ WIP: feature(addon-google-analytics) utilise storybook addonParameter instead of window/global scope. Jun 27, 2020

shilman added addon: analytics BREAKING CHANGE labels Jun 27, 2020

Fix typescript errors, remove unused global dependency

9326453

alexanderjeurissen requested review from ndelangen, shilman and stijnkoopal as code owners June 27, 2020 13:11

alexanderjeurissen commented Jun 27, 2020

View reviewed changes

alexanderjeurissen changed the title ~~WIP: feature(addon-google-analytics) utilise storybook addonParameter instead of window/global scope.~~ feature(addon-google-analytics) utilise storybook addonParameter instead of window/global scope. Jun 27, 2020

Make parameter changes backwards compatible.

237d3d9

update README.

77a1297

alexanderjeurissen mentioned this pull request Jun 27, 2020

feature(addon-google-analytics) Make tracking of specific storybook core events configurable #11340

Closed

ndelangen reviewed Jul 3, 2020

View reviewed changes

stale bot added inactive and removed inactive labels Aug 16, 2020

ActuallyACat mentioned this pull request Aug 18, 2020

ci: Added a gtm plugin to manage analytics cultureamp/kaizen-design-system#714

Merged

stale bot added the inactive label Sep 7, 2020

stale bot closed this Oct 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(addon-google-analytics) utilise storybook addonParameter instead of window/global scope. #11336

feature(addon-google-analytics) utilise storybook addonParameter instead of window/global scope. #11336

alexanderjeurissen commented Jun 27, 2020 •

edited

Loading

shilman commented Jun 27, 2020 •

edited

Loading

alexanderjeurissen commented Jun 27, 2020 •

edited

Loading

alexanderjeurissen commented Jun 27, 2020

shilman commented Jun 27, 2020

alexanderjeurissen commented Jun 27, 2020

alexanderjeurissen Jun 27, 2020

alexanderjeurissen commented Jun 27, 2020

shilman commented Jun 27, 2020

shilman commented Jun 27, 2020

alexanderjeurissen commented Jun 27, 2020 •

edited

Loading

alexanderjeurissen commented Jun 27, 2020

alexanderjeurissen commented Jun 30, 2020

shilman commented Jun 30, 2020

ndelangen Jul 3, 2020

alexanderjeurissen Jul 3, 2020

ndelangen Jul 6, 2020

alexanderjeurissen Jul 6, 2020 •

edited

Loading

alexanderjeurissen Jul 10, 2020

alexanderjeurissen Jul 25, 2020 •

edited

Loading

ndelangen Aug 17, 2020

stale bot commented Aug 16, 2020

stale bot commented Sep 7, 2020

stale bot commented Oct 12, 2020

feature(addon-google-analytics) utilise storybook addonParameter instead of window/global scope. #11336

feature(addon-google-analytics) utilise storybook addonParameter instead of window/global scope. #11336

Conversation

alexanderjeurissen commented Jun 27, 2020 • edited Loading

What I did

How to test

shilman commented Jun 27, 2020 • edited Loading

alexanderjeurissen commented Jun 27, 2020 • edited Loading

alexanderjeurissen commented Jun 27, 2020

shilman commented Jun 27, 2020

alexanderjeurissen commented Jun 27, 2020

alexanderjeurissen Jun 27, 2020

Choose a reason for hiding this comment

alexanderjeurissen commented Jun 27, 2020

shilman commented Jun 27, 2020

shilman commented Jun 27, 2020

alexanderjeurissen commented Jun 27, 2020 • edited Loading

alexanderjeurissen commented Jun 27, 2020

alexanderjeurissen commented Jun 30, 2020

shilman commented Jun 30, 2020

ndelangen Jul 3, 2020

Choose a reason for hiding this comment

alexanderjeurissen Jul 3, 2020

Choose a reason for hiding this comment

ndelangen Jul 6, 2020

Choose a reason for hiding this comment

alexanderjeurissen Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

alexanderjeurissen Jul 10, 2020

Choose a reason for hiding this comment

alexanderjeurissen Jul 25, 2020 • edited Loading

Choose a reason for hiding this comment

ndelangen Aug 17, 2020

Choose a reason for hiding this comment

stale bot commented Aug 16, 2020

stale bot commented Sep 7, 2020

stale bot commented Oct 12, 2020

alexanderjeurissen commented Jun 27, 2020 •

edited

Loading

shilman commented Jun 27, 2020 •

edited

Loading

alexanderjeurissen commented Jun 27, 2020 •

edited

Loading

alexanderjeurissen commented Jun 27, 2020 •

edited

Loading

alexanderjeurissen Jul 6, 2020 •

edited

Loading

alexanderjeurissen Jul 25, 2020 •

edited

Loading