search

google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.22k stars 277 forks source link

Update UA and GA4 dropdowns behavior to only show once parent is specified #3243

Closed felixarntz closed 3 years ago

felixarntz commented 3 years ago

Currently, the Analytics dropdowns for account, property and profile/view always display, with the latter two being disabled until the respective parent entity has been specified. In relation to the upcoming GA4 setup flow, this should be modified so that the user only sees the dropdowns once they actually allow a selection to be made. This should be changed for the existing infrastructure though too, i.e. it will affect Site Kit production behavior already today.

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation Brief

For the three dropdowns:

Return null if there is no account ID

And also for the Analytics (UA) ProfileSelect

Return null is there is no property ID

Storybook changes

The Analytics section has two stories: Account Property Profile Select (none selected) and Account Property Profile Select (all selected)

Test Coverage

No existing tests currently fail because of the above change

Add extra tests for the three dropdowns to assert that null is returned when

Visual Regression Changes

Checked and there should be no visual regression changes with above changes

QA Brief

The UA PropertySelect and the ProfileSelect changes can easily be seen on storybook https://google.github.io/site-kit-wp/storybook/pull/3330/?path=/story/analytics-module--account-property-profile-select

Here you can observe the changes are made as per the AC

Also they can be seen in WordPress but as we have a few flows, to get an empty account I had to select to create an account, then go back (as IIRC the account is being prefilled)

Changelog entry

eugene-manuilov commented 3 years ago

@danielgent we also need to mention storybook changes requested in the AC. Could you please update the IB?

danielgent commented 3 years ago

Sure thing @eugene-manuilov ! Apologies, I forgot that Backstop uses only the Storybook stories that we configure :smile:

I'll manually check storybook now

danielgent commented 3 years ago

Hey @eugene-manuilov .

I was thinking there would be more instances of this in StoryBook (I thought it would show all the difference states just for the PropertySelect)

But actually these are not used in many places on storybook

There is a new GA4 section that just shows the GA4 PropertySelect (no change here)

But then the Analytics section has Account Property Profile Select (none selected) and Account Property Profile Select (all selected)

The latter isn't changed but the former now just shows one select (as expected!)

now-shows-disabled-state now-only-shows-one

Ideally what I'd want is this to be fully interactive so it can be seen working, but what do you think? It's kind of fine how it is, or at least no more broken than it currently is, as interacting with the selects leads to exceptions! exception

eugene-manuilov commented 3 years ago

Ideally what I'd want is this to be fully interactive so it can be seen working, but what do you think? It's kind of fine how it is, or at least no more broken than it currently is, as interacting with the selects leads to exceptions!

@danielgent yes, it would be awesome if we can make it fully interactive. It will require a bit of work to properly configure datastores though. In this case, we will be able to merge two stories (none selected and all selected) into one and maybe even convert it to storybook 6 :thinking:

Could you please update IB to mention changes required to update those stories?

danielgent commented 3 years ago

@eugene-manuilov I've run out of time this week, but I can keep on this ticket next week :+1:

I read up for a bit and I'm clearer now on Storybook 6 and Component Story Format. I haven't used it for a few years and there have been big changes recently (with all the add-ons I used to use being deprecated :smile: )

I'm not sure what we do in Storybook though for components that do network requests. I'll look through all the stories on Monday though and see if I can find an example :+1:

eugene-manuilov commented 3 years ago

@danielgent we just pre-set values that are going to be received via network using receive* actions. For instance, you can see it here: https://github.com/google/site-kit-wp/blob/8c341d37f0ba40d83ce5a8a067ac223f42c4f9d8/stories/module-analytics-settings.stories.js#L95-L111

danielgent commented 3 years ago

@eugene-manuilov Thanks. That's really easy actually. No depdency injection of API classes or anything like that :smile:

I've just had a play around then locally and I got something working.

danielgent commented 3 years ago

@eugene-manuilov I don't think I can write an IB for converting to storybook 6 because this PR has been open for a while, and it makes large changes. There is some discussion around our new best practices for storybook 6 https://github.com/google/site-kit-wp/pull/3230

eugene-manuilov commented 3 years ago

IB :heavy_check_mark:

eugene-manuilov commented 3 years ago

@wpdarren I found one issue with the profile select dropdown. I'll create a follow-up PR to fix it.

wpdarren commented 3 years ago

QA Update: Pass ✅

Tested this on Storyboard here

Verified: