-
Notifications
You must be signed in to change notification settings - Fork 293
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
Provide link in Analytics settings panel to update GA4 configuration and improve UI for related UA link #3702
Comments
@Pratheep-lab I've created a task in Asana for this https://app.asana.com/0/1185360131189553/1200629922083182 where we can handle the UX recommendations. |
@marrrmarrr I defined ACs based on what we had discussed 1-2 weeks ago, let's for now display two short "Edit in Analytics" links right next to the Analytics IDs. cc @Pratheep-lab @eclarke1 This should get a UX review during the implementation, e.g. using Storybook. |
@danielgent, please, avoid using "PR as IB" approach. I know it's a big temptation to start coding instead of writing IB but we need to write IB first because sometimes things that seem obvious may change during IB reviews. We can use "PR as IB" approach only for edge cases when we found something small that needs to be squeezed into the current release. But for regular tickets, we should go with our main procedure and create comprehensive IBs. Please, update this ticket to have standard IB. You can leave a link to your PR at the end of IB. |
The AC is already a detailed list of what steps to do I'm just going to paste the code into the IB then 🤷 I think I understand the concept of an IB for complex tickets (although I can't find other people doing this online) but this diff is tiny |
I've just added an IB. Either I've done it totally wrong, or I'm missing something, because it's way easier to read my annotated PR and look at the code, than read those bullet points 😄 It reminds me of podcasts trying to explain code and them lamenting that they could show it on a video much easier |
@danielgent could you, please, be more specific? What do you mean by "Build the URL for this"? Which link do you mean when you say "Move this link up into the same div..."? IB needs to be clear, so everyone can understand what we need to do there.
When we compose URLs using string templates, we need to prepend the template with the
We need to wrap the link with
We need to hide these |
@felixarntz, as far as I remember, we didn't want users to face the "web data stream" terminology in our UI. Did we change our minds? |
Yes, sometimes AC contains too many technical details. But it doesn't mean that there is nothing to add to it in IB. We can refer to specific sections of AC when we write IB, but AC shouldn't be a replacement for our IB. |
@felixarntz here is a follow-up PR: #3864. Do you mind reviewing it? |
It's not aligned though @felixarntz @eugene-manuilov @aaemnnosttv 😄 On my original approach in the IB I used empty headers as a hack so that it all lined up |
@felixarntz I was going to mention the spacing between the sentence and link but thought I was being too picky! 😃 It does look a lot better but I agree with @danielgent though it doesn't look aligned. Shall we open up a new ticket for that, unless we want to change this before the release? |
@wpdarren @danielgent @eugene-manuilov @aaemnnosttv Let's try to fix it before the release on Monday, but without any hacks please :) We may need to ensure the line height is the same despite the smaller font size? Alternatively, let's change the font size back to what it originally was (i.e. remove the |
@felixarntz do you mind reviewing #3874? |
@eugene-manuilov This now looks good to go on both mobile and desktop after #3874. |
In the Analytics settings panel (in view-only state), there is currently a long message with a link that points to the Analytics frontend for the currently active Analytics property and its view.
Now that users can also have a GA4 property configured there, this message is confusing, as it appears below all of those settings while still only focusing on UA. We will now need two links instead of just the one. In addition, it's a bit strange to have a long sentence as a complete link anyway. Let's revisit this UI.
Screenshot:
A potential solution would be:
cc @Pratheep-lab
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
In the Analytics settings view:
https://analytics.google.com/analytics/web/?authuser={googleEmailAddress}#/a{accountID}w{uaPropertyID}p{viewID}/admin/view/settings
(same as the previous "You can make changes to this view..." link)https://analytics.google.com/analytics/web/?authuser={googleEmailAddress}#/a{accountID}p{ga4PropertyID}/admin/streams/table/{webDataStreamID}
Implementation Brief
Using
assets/js/modules/analytics/components/settings/SettingsView.js
In order to build the GA4 Url
Get the
webDataStreamID
fromselect( MODULES_ANALYTICS_4 ).getWebDataStreamID()
Build the Url like
editViewSettingsURL
but with the same Url structure as the AC (passing this intogetServiceURL
)Update the text for the existing UA link as per the AC
These links should be encoded with
escapeURI
(requires updating existing UA link).googlesitekit-settings-module__meta-items
as containing the UA info.googlesitekit-settings-module__meta-items
of the GA4 infoIn order to line up the text for both these links
<h5>
s at the top of each Link'sgooglesitekit-settings-module__meta-item
(just containing a
).<h5>
s should be hidden on mobileTo style the Links to be the same size as the rest of the text
.googlesitekit-settings-module__inline-link
to the Links (through the prop className)assets/sass/components/settings/_googlesitekit-settings-module.scss
Test Coverage
Visual Regression Changes
QA Brief
Changelog entry
The text was updated successfully, but these errors were encountered: