Implement the loading state for the Audience Tile. #9030
Conversation
|
Build files for 2b21f2d have been deleted. |
|
Size Change: +17.7 kB (+1.07%) Total Size: 1.67 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Hi @benbowler, nice work here so far. There are a few areas that could use an update, and I've also flagged that optional change we discussed for your consideration.
| <PreviewBlock width="100%" height="14px" /> | ||
| <PreviewBlock width="100%" height="52px" /> | ||
| <PreviewBlock width="100%" height="52px" /> | ||
| <PreviewBlock width="100%" height="52px" /> | ||
| <PreviewBlock width="100%" height="52px" /> | ||
| <PreviewBlock width="100%" height="52px" /> | ||
| <PreviewBlock width="100%" height="52px" /> |
There was a problem hiding this comment.
At present the preview blocks don't match up too well in desktop viewports:
In practical terms this tends to result in a layout shift as all tiles tend to be in the loading state at the same time, so the initial height is derived from the loading state.
There's also a bit of a mismatch in mobile viewports, as the top preview block no longer corresponds to the title.
[Note that we do have a bug raised for the broken mobile layout, #8930), and don't need to consider that here.]
It would be good to avoid rendering that top preview block for mobile viewports, and to make use of the breakpoint-specific width props to better match the preview blocks to the loaded state.
site-kit-wp/assets/js/components/PreviewBlock.js
Lines 42 to 47 in 0f74918
There was a problem hiding this comment.
Thanks Tom, my initial approach was to match the designs. I've updated these checking there is no layout shift on the zero data state, I could increase this to match the full data state if you think that would be better, LMK.
| // Remove any configuredAudiences that are no longer available in availableAudiences. | ||
| const configuredAudiences = | ||
| select( MODULES_ANALYTICS_4 ).getConfiguredAudiences(); | ||
|
|
||
| const newConfiguredAudiences = configuredAudiences?.filter( | ||
| ( configuredAudience ) => | ||
| availableAudiences?.some( | ||
| ( { name } ) => name === configuredAudience | ||
| ) | ||
| ); | ||
| dispatch( MODULES_ANALYTICS_4 ).setConfiguredAudiences( | ||
| newConfiguredAudiences || [] | ||
| ); |
There was a problem hiding this comment.
As discussed on the call earlier, we should move this block that updates configuredAudiences to the syncAvailableAudiences() action, as we want to ensure the list of configured audiences is updated any time we do a sync.
|
|
||
| const settings = { | ||
| configuredAudiences: availableAudiencesFixture.reduce( | ||
| ( acc, a ) => [ ...acc, a.name ], |
There was a problem hiding this comment.
Assuming this line is kept what with the requested change to maybeSyncAvailableAudiences():
Please avoid abbreviated variable names as per our naming conventions (I know we do tend to use acc so let's let that one slide, but please rename a or simply destructure name).
| ( acc, a ) => [ ...acc, a.name ], | |
| ( acc, audience ) => [ ...acc, audience.name ], |
| .getConfiguredAudiences() | ||
| ).toEqual( | ||
| availableAudiencesSubset.reduce( | ||
| ( acc, a ) => [ ...acc, a.name ], |
There was a problem hiding this comment.
As above:
| ( acc, a ) => [ ...acc, a.name ], | |
| ( acc, audience ) => [ ...acc, audience.name ], |
There was a problem hiding this comment.
audience is already declared in the upper scope so I will go with destructuring.
| ); | ||
| } ); | ||
|
|
||
| it( 'should not call syncAvailableAudiences if the availableAudiencesLastSyncedAt is within the last hour', async () => { |
There was a problem hiding this comment.
Minor typo/grammatical error:
| it( 'should not call syncAvailableAudiences if the availableAudiencesLastSyncedAt is within the last hour', async () => { | |
| it( 'should not call syncAvailableAudiences if the availableAudiencesLastSyncedAt setting is within the last hour', async () => { |
| @@ -304,6 +307,107 @@ describe( 'modules/analytics-4 audiences', () => { | |||
| } ); | |||
| } ); | |||
|
|
|||
| describe( 'maybeSyncAvailableAudiences', () => { | |||
| it( 'should call syncAvailableAudiences if the availableAudiencesLastSyncedAt setting is undefined', async () => { | |||
| fetchMock.post( syncAvailableAudiencesEndpoint, { | |||
There was a problem hiding this comment.
We tend to prefer postOnce() over post() where we can use it, as it's more specific and will fail if an unexpected post occurs:
| fetchMock.post( syncAvailableAudiencesEndpoint, { | |
| fetchMock.postOnce( syncAvailableAudiencesEndpoint, { |
| } ); | ||
|
|
||
| it( 'should call syncAvailableAudiences if the availableAudiencesLastSyncedAt is not within the last hour', async () => { | ||
| fetchMock.post( syncAvailableAudiencesEndpoint, { |
There was a problem hiding this comment.
As above:
| fetchMock.post( syncAvailableAudiencesEndpoint, { | |
| fetchMock.postOnce( syncAvailableAudiencesEndpoint, { |
|
|
||
| .googlesitekit-plugin { | ||
| .googlesitekit-audience-segmentation-tile-loading { | ||
| margin: 24px 24px 28px; |
There was a problem hiding this comment.
This should use $grid-gap-phone with a media rule for desktop viewports. Please take a look at the + 4px for mobile as well, that may need adjusting.
| margin: 24px 24px 28px; | |
| margin: $grid-gap-phone $grid-gap-phone $grid-gap-phone + 4px; | |
| @media (min-width: $bp-desktop) { | |
| margin: $grid-gap-desktop $grid-gap-desktop $grid-gap-desktop + 4px; | |
| } |
| margin: 12px 0; | ||
|
|
||
| &:first-of-type { | ||
| margin-bottom: 24px; |
There was a problem hiding this comment.
Please check to see if this should also be a responsive value using $grid-gap-phone / $grid-gap-desktop.
There was a problem hiding this comment.
As discussed on the call earlier, it would be good to address the "flickering" effect when changing the list of selected audiences and so forth, where currently the tiles disappear but should remain visible in a loading state.
By the look of it, removing the check for reportLoaded from this while condition should do the trick:
That said I haven't extensively tested it. So, if you want to give that a look and feel like including it in this PR, please do, but if you feel it might start pushing this over the estimate, what with the other changes I've requested, then let me know and we can create a separate issue for it.
There was a problem hiding this comment.
I implemented as above, if you can let me know how to repeat the flickering I can test more, with this change I was not seeing flickering.
There was a problem hiding this comment.
Thanks @benbowler - I'm no longer seeing it either with this change. Let's merge this and keep an eye on things, hopefully it's done the trick.
There was a problem hiding this comment.
Hi @benbowler, I've spotted one more thing.
Unfortunately the way the No Audiences Banner has been implemented is a bit at odds with the changes being introduced via this issue.
Note how the NoAudienceBannerWidget widget is only considered active when there is at least one configured audience:
site-kit-wp/assets/js/modules/analytics-4/index.js
Lines 149 to 158 in 0f74918
Similarly the logic in NoAudienceBannerWidget depends on configuredAudiences being populated:
We're going to need to change the No Audiences Banner so there is only a dependency on configuredAudiences being set i.e. non-null rather than being populated, because we are now trimming configuredAudiences when we sync the audiences, so configuredAudiences may end up completely empty - a condition where the No Audiences Banner should be displayed, but currently won't be.
Again - this might be a bit much to fit into this issue - please let me know if you feel it can be addressed here, otherwise this would be fair game for a followup issue.
|
Thanks @techanvil, to you final point, I believe I've addressed in this commit, I kept it clean so those changes can be reviewed and updated independently to get it right. Let me know if I missed anything here. |
Summary
Addresses issue:
Relevant technical choices
I implemented a new loading component and scss file to prevent the AudienceTile component from getting too large.
One think I noticed while working on this ticket, is that the height of the loading tile (551px) is different to other states of the tile, such as the fully loaded data state (604px). This will cause layout shift once the loading state completes. I don't think this is a major issue, also it's hard to avoid due to the differences in height of the tiles based on partial data state etc, but wanted to raise it here.
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