Improve experience with recoverable modules for view-only users. #5465
Conversation
…combineWidgets().
|
Size Change: +2.59 kB (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks @techanvil, this looks and works great! Just a few minor things to address and this should be good to go. Nice work!
| setupRegistry: setupRegistry( { | ||
| recoverableModules: [ 'search-console' ], | ||
| } ), | ||
| viewContext: VIEW_CONTEXT_DASHBOARD_VIEW_ONLY, |
There was a problem hiding this comment.
Let's also make sure the inverse of this is covered – in a non-view-only context widgets for recoverable modules should be output as normal.
There was a problem hiding this comment.
Thanks, good shout - I've added another test case.
| // Here we serialize to `moduleSlug` for compatibility with the logic in | ||
| // `combineWidgets()`. In future we may wish to take a less "hacky" approach. | ||
| // See https://github.com/google/site-kit-wp/issues/5376#issuecomment-1165771399. | ||
| moduleSlug: JSON.stringify( moduleSlugs.sort() ), |
There was a problem hiding this comment.
We shouldn't call sort on the moduleSlugs prop directly because it will mutate the input.
Also, let's use join instead of stringify because the result for a module that only relies on a single module will be only the one module's slug rather than a JSON string of the array.
| moduleSlug: JSON.stringify( moduleSlugs.sort() ), | |
| moduleSlug: [ ...moduleSlugs ].sort().join(), |
There was a problem hiding this comment.
Thanks, good spot on the sort mutation!
I've fixed that and used join rather than JSON.stringify. I joined using a comma to make the value a bit more coherent and avoid an obscure edge case...
There was a problem hiding this comment.
Great thanks! join() uses a comma by default but good to be explicit I suppose. You can also do [1,2,3].toString() which will be comma-separated as well 😉
There was a problem hiding this comment.
Well, thanks for another reminder of the basics. Mutable sort was an oversight on my part, but I had completely forgotten join defaults to a comma! Regardless, I do prefer the explicit version (probably evident from the fact I had forgotten about the default), for the sake of, well, being explicit. :)
| /** | ||
| * External dependencies | ||
| */ | ||
| import { getByText } from '@testing-library/dom'; |
There was a problem hiding this comment.
This shouldn't be necessary as this query is returned by render
There was a problem hiding this comment.
This is a deliberate choice, as I want to use the version of getByText which accepts a container as the first argument, rather than the bound version returned by render.
There was a problem hiding this comment.
Thanks! I'm aware of the difference, the part that isn't obvious is why this would be intentionally used/needed instead?
There was a problem hiding this comment.
Well, in the test where it's used, I am asserting that the visible widget contains the text rendered by RecoverableModules:
If I use the bound version and omit the container, this test fails, because RecoverableModules is in fact rendered three times into the DOM, twice within the scope of a .googlesitekit-hidden class (as evident in the corresponding snapshot), and getByText throws an error if there is more than one match for the text.
So, I want to use the non-bound version with the container to scope getByText to the visible widget.
There are of course other ways to code this assertion, but this seemed good enough to me at the time of writing the test...
| Object.values( modules ).reduce( ( recoverable, module ) => { | ||
| if ( recoverableModules.includes( module.slug ) ) { | ||
| return { | ||
| ...recoverable, | ||
| [ module.slug ]: module, | ||
| }; | ||
| } | ||
|
|
||
| return recoverable; | ||
| }, {} ) |
There was a problem hiding this comment.
This would be more efficient to iterate over the recoverable slugs as there will always be less of these than all the modules (simply because not all modules are shareable). We could also then check if modules[recoverableSlug] rather than the list search when reducing.
The sets are so small that this probably doesn't matter at all but worth mentioning.
There was a problem hiding this comment.
Yeah, this did cross my mind too. As I was simply moving this code, I thought it might cloud the issue if I also rewrote it. Besides, as you've pointed out the sets are small, and likely to stay that way, so it would arguably be a case of premature optimization to rewrite it...
Co-authored-by: Evan Mattson <emattson@google.com>
|
Thanks @aaemnnosttv, I have addressed your comments 👍 |
Summary
Addresses issue:
Relevant technical choices
recoverableModulesfeature flag being enabled to avoid callinggetRecoverableModules()when the feature is not enabled and logging an error as a result.getRecoverableModulesin order to help prevent an issue with unwanted rerendering.moduleSlugsasmetadata.moduleSlugin order to play nicely with the existing logic incombineWidgets() -> shouldCombineAllWidgets().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