-
Notifications
You must be signed in to change notification settings - Fork 908
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
Add Organic Sessions widget #22073
base: trunk
Are you sure you want to change the base?
Add Organic Sessions widget #22073
Conversation
Pull Request Test Coverage Report for Build 7df84ae28a5f78d3bb0730712ce6aeadd7b6fa84Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch. |
fix title fix title remove console.log
change the skeleton and chart size
In there is no daily data there wouldn't be change data.
gradient size
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.
CR 🚧 some questions and feedback
@@ -98,6 +100,16 @@ export class WidgetFactory { | |||
remoteDataProvider={ this.#remoteDataProvider } | |||
dataFormatter={ this.#dataFormatter } | |||
/>; | |||
case WidgetFactory.types.organicSessions: | |||
if ( ! isFeatureEnabled || ! isConnected ) { |
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 needs to be a GA 4 connected check as well not just connected. It should be in the script data as isGAConnected
const current = data?.current?.sessions || 0; | ||
const previous = data?.previous?.sessions || 0; | ||
// Delta / average. | ||
const difference = Math.abs( current - previous ) / ( ( current + previous ) / 2 ); |
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.
Would it make sense to have an early return here if both current && previous end up 0?
* @param {DataFormatter} dataFormatter The data formatter. | ||
* @returns {{data: *, error: Error, isPending: boolean}} The remote data info. | ||
*/ | ||
export const useOrganicSessionsChange = ( dataProvider, remoteDataProvider, dataFormatter ) => { |
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.
I think this might need a bit of a comment describing what happens here, because to me it seems a lot is happening here.
* @param {DataProvider} dataProvider The data provider. | ||
* @param {RemoteDataProvider} remoteDataProvider The remote data provider. | ||
* @param {DataFormatter} dataFormatter The data formatter. | ||
* @returns {{data: *, error: Error, isPending: boolean}} The remote data info. |
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.
Shoud data: * not be data: our defined response?
* @returns {function(?RawOrganicSessionsChangeData[]): OrganicSessionsChangeData} Function to format the organic sessions change data. | ||
*/ | ||
// eslint-disable-next-line complexity -- Fallbacks to zero, easy enough to read. | ||
export const createOrganicSessionsChangeFormatter = ( dataFormatter ) => ( [ data ] ) => { |
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.
I see the word Change
everywhere and in the Back-end
we use Compare
everywhere for these different comparisons between a previous period and the current one. Lets choose one and stick to it.
* @param {DataFormatter} dataFormatter The data formatter. | ||
* @returns {function(?OrganicSessionsDailyData[]): OrganicSessionsDailyData[]} Function to format the organic sessions daily data. | ||
*/ | ||
export const createOrganicSessionsDailyFormatter = ( dataFormatter ) => ( data = [] ) => data.map( ( item ) => ( { |
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.
Is this export needed?
* @returns {function(?RawOrganicSessionsChangeData[]): OrganicSessionsChangeData} Function to format the organic sessions change data. | ||
*/ | ||
// eslint-disable-next-line complexity -- Fallbacks to zero, easy enough to read. | ||
export const createOrganicSessionsChangeFormatter = ( dataFormatter ) => ( [ data ] ) => { |
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.
Is the export needed?
* @param {DataFormatter} dataFormatter The data formatter. | ||
* @returns {{data: *, error: Error, isPending: boolean}} The remote data info. | ||
*/ | ||
export const useOrganicSessionsDaily = ( dataProvider, remoteDataProvider, dataFormatter ) => { |
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.
Same comments as with the other method might benefit from a little explanation what this all does?
*/ | ||
const getOrganicSessionsDaily = useCallback( ( options ) => { | ||
return remoteDataProvider.fetchJson( | ||
dataProvider.getEndpoint( "timeBasedSeoMetrics" ), |
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.
We might want to also define all endpoints as a constant somewhere since we have quite a few random strings all over the place
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
offline
. Change the tab toAlert center
and go back to the Dashboard.Relevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Other environments
[shopify-seo]
, added test instructions for Shopify and attached theShopify
label to this PR.Documentation
Quality assurance
Innovation
innovation
label.Fixes https://github.com/Yoast/reserved-tasks/issues/411