Feature/9217 top traffic source driving leads #9320
Conversation
|
Size Change: +726 B (+0.04%) Total Size: 1.85 MB
ℹ️ View Unchanged
|
|
Build files for 66761b3 have been deleted. |
|
Note to the reviewer: The E2E tests are failing for reasons not related to this PR, but they originate from different issue that was merged, and causes E2E failures in all PR's Update: I pushed the latest sync with developed, and removed |
| const loading = useSelect( ( select ) => | ||
| eventNames.length | ||
| ? ! select( MODULES_ANALYTICS_4 ).hasFinishedResolution( | ||
| 'getReport', | ||
| [ totalLeadsReportOptions ] | ||
| ) || | ||
| ! select( MODULES_ANALYTICS_4 ).hasFinishedResolution( | ||
| 'getReport', | ||
| [ trafficSourceReportOptions ] | ||
| ) | ||
| : undefined | ||
| ); |
There was a problem hiding this comment.
The ternary with the || is very tough to read. In general, multi-line ternary statements aren't great, but this one is overkill 😅
Let's change it to return early with if statements and simplify it at least a bit:
| const loading = useSelect( ( select ) => | |
| eventNames.length | |
| ? ! select( MODULES_ANALYTICS_4 ).hasFinishedResolution( | |
| 'getReport', | |
| [ totalLeadsReportOptions ] | |
| ) || | |
| ! select( MODULES_ANALYTICS_4 ).hasFinishedResolution( | |
| 'getReport', | |
| [ trafficSourceReportOptions ] | |
| ) | |
| : undefined | |
| ); | |
| const loading = useSelect( ( select ) => { | |
| if ( ! eventNames.length ) { | |
| return undefined; | |
| } | |
| return ! select( MODULES_ANALYTICS_4 ).hasFinishedResolution( | |
| 'getReport', | |
| [ totalLeadsReportOptions ] | |
| ) || | |
| ! select( MODULES_ANALYTICS_4 ).hasFinishedResolution( | |
| 'getReport', | |
| [ trafficSourceReportOptions ] | |
| ) | |
| } ); |
There was a problem hiding this comment.
Good point, updated
| const makeFilter = ( dateRange, dimensionIndex ) => ( row ) => | ||
| get( row, `dimensionValues.${ dimensionIndex }.value` ) === dateRange; |
There was a problem hiding this comment.
The multiple implicit returns make this hard to follow/tell what's what in terms of the functions this is creating. How about:
| const makeFilter = ( dateRange, dimensionIndex ) => ( row ) => | |
| get( row, `dimensionValues.${ dimensionIndex }.value` ) === dateRange; | |
| const makeFilter = ( dateRange, dimensionIndex ) => { | |
| return ( row ) => { | |
| return get( row, `dimensionValues.${ dimensionIndex }.value` ) === dateRange | |
| } | |
| }; |
There was a problem hiding this comment.
This isn't an error state specific to this widget, so let's remove this test. We already have a lot of VRTs and the size is often causing them to fail/timeout, plus it takes longer to run for every test we add. When they add value that's worth it, but this one doesn't add much, so let's remove it 🙂
There was a problem hiding this comment.
This isn't an error state specific to this widget, so let's remove this test. We already have a lot of VRTs and the size is often causing them to fail/timeout, plus it takes longer to run for every test we add. When they add value that's worth it, but this one doesn't add much, so let's remove it 🙂
There was a problem hiding this comment.
Not sure this one is worth a VRT either, this can probably be removed 🙂
There was a problem hiding this comment.
Even this is covered by other VRTs/the base KMW VRTs, so I'd say this is probably safe to remove as well.
I think our amount of VRTs is probably overkill 😅
There was a problem hiding this comment.
I kept this one only, as it still handy to have it, so we can see if something goes wrong with it, as although it displays data similarly to the base traffic metric, it still has different options that are needed in order to work.
…raffic-source-driving-leads.
…b.com:google/site-kit-wp into feature/9217-top-traffic-source-driving-leads.
|
Thanks @tofumatt I updated the PR, although I removed |
|
Usually you need to run |
Summary
Addresses issue:
Top traffic source driving leadsACR KMW #9217Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist