-
Notifications
You must be signed in to change notification settings - Fork 361
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
change: [M3-8439] - Tag form events in Linode Create v2 #10840
change: [M3-8439] - Tag form events in Linode Create v2 #10840
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR adds form events that were in LCv1 but missing in LCv2. It does not go back and clean up LCv1 form events because those files will presumably be removed soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed unused form events from this file because this component not just part of the Linode Create flow v1 (i.e. it will not be deleted soon).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added test coverage for the 'How Data Center Pricing Works' docs link because it was missing from the TwoStepRegion select before.
@@ -134,6 +143,38 @@ export const LinodeCreatev2 = () => { | |||
} | |||
}; | |||
|
|||
const handleAnalyticsFormError = useCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ported over from LinodeCreateContainer and adapted slightly for how errors in LCv2 are handled. UX wants to track specific errors for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: Could we extract out some of this logic into a hook or function to keep this file a bit cleaner?
I just don't want this file to spiral out of control and resemble Linode Create v1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, fair point, we don't want to creep back to LCv1. I moved this to a hook in bd72f50, though wasn't sure how best to unit test.
Coverage Report: ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! seeing most events firing with proper tags for each create mode. ✅
A couple things are not firing on my end
- region selection
- plan selection
Am i missing something?
packages/manager/src/features/Linodes/LinodeCreatev2/Actions.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Linodes/LinodeCreatev2/Details/PlacementGroupPanel.tsx
Outdated
Show resolved
Hide resolved
@@ -134,6 +143,38 @@ export const LinodeCreatev2 = () => { | |||
} | |||
}; | |||
|
|||
const handleAnalyticsFormError = useCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: Could we extract out some of this logic into a hook or function to keep this file a bit cleaner?
I just don't want this file to spiral out of control and resemble Linode Create v1
@abailly-akamai Thanks for verifying! For Region selection, you should be seeing the logged formStart event, even if the second message says something like "criteria not met". The first message is sufficient for confirming the event in CM; the second message has to do with whether this url has been configured for the formStart in the analytics backend. (I think they turned formStart events off on localhost to save some event calls when we went to prod with LCv1 last week.) For Plan selection, you should not be seeing any form events, but you should be seeing a formInput event upon clicking the "Choosing a Plan" docs link. (There are actually two analytics events here; a custom event and a form event. Form event is in orange below.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good on my end!
Nice job keeping things as clean as possible 🧼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx for the clarifications! PR is good to go from my end, thx for tackling this!
I'm not seeing Firewall change/clear events 🤔 |
@mjac0bs Yes, I'm seeing all other events Screen.Recording.2024-08-27.at.12.30.36.PM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out my Linode Create v2 flag was off 😅 . Seeing all the events now!
Description 📝
This PR tags the UI elements from Linode Create v2 that were not shared with the v1 flow (which was tagged in #10722).
Changes 🔄
See #10722 for an overview how form events are handled.
Target release date 🗓️
9/3
Preview 📷
Screenshots of Main Create Flow Events
Screenshots of PG Branch Flow Events
Screenshots of Firewall Branch Flow Events
Screenshots of VPC Branch Flow Events
How to test 🧪
Prerequisites
(How to setup test environment)
.env
.yarn dev
, and in the browser console, type_satellite.setDebug(true)
.Verification steps
(How to verify changes)
As an Author I have considered 🤔
Check all that apply