-
Notifications
You must be signed in to change notification settings - Fork 286
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
Decouple module-specific effects from core components #8211
Comments
One more report of this error in the support forums. Awaiting user feedback. |
Thanks, @zutigrm, but please do not use the PR as IB approach because it is strongly discouraged. If you can't write an IB without coding it beforehand, then just leave it for someone else. By spending time on your PoC, you spend time on implementation in the current sprint for a thing that is supposed to be implemented in a following sprint, thus our planning for that sprint will less accurate. Plus it is not always the case that PoC will be needed, thus time spent on it will be wasted.
This is a nice start, but I don't think it should be done this way. The problem here is that you try to squeeze module related functionality into the widgets domain that should know nothing about that. We need to separate concerns. The more appropriate solution would be updating the Using components approach will let us perform side effects that rely on certain criteria to happen and can be deferred in time.
Looks like there is no |
@eugene-manuilov Thanks for the feedback/clarification and new route suggestion. I updated IB to reflect that approach. |
Thanks, @zutigrm. Mostly looks good.
Let's put it into the
We need to update the
|
@eugene-manuilov Thanks, IB updated |
Thanks, @zutigrm. IB ✔️ |
This is good to go now but we should only merge it after the split for the release. Assigning to @tofumatt to do that afterwards 👍 |
Just a note to mention there are a couple more reports of this users impacted by this over the past few days. Details below: |
QA Update ✅
Recording.1201.mp4Recording.1202.mp4Recording.1203.mp4Recording.1204.mp4Recording.1205.mp4Recording.1206.mp4Recording.1207.mp4Recording.1208.mp4Custom dimension Recording.1231.mp4Google tag sync Recording.1232.mp4 |
Feature Description
As a general principal, module-specific code can depend on core infrastructure, but not the other way around. Core infra being core data stores as one of the most explicit examples, but also things like very top-level components like
DashboardMainApp
.The SK dashboards have a well-defined API for adding elements to the dashboard but the concept of widgets is directly associated with layout. Sometimes we want to invoke various module-specific side-effects which in the past have either been shoe-horned into an existing component, or added into a higher level component where they don't really belong.
There have been a few instances recently where we've wanted to use such functionality where it hasn't been available such as synchronizing module settings and creating custom dimensions to name a few.
One solution to this is to use the Widgets API – even today this can be used to accomplish the end goal.
Example
This registers a widget on the main dashboard for the sole purpose of running effects. This will work, and by using
WidgetNull
unconditionally, the layout will never be affected by the "empty" widget. This could be combined with thewhenActive()
HOC for guarding it to only run when a module is active or connected. It isn't entirely ideal as-is here as there are things likewidth
that aren't relevant, nor should we need to renderWidgetNull
to avoid throwing a wrench in the layout when it isn't concerned with layout at all.One idea is to register a new widget area that would always be hidden which could be used for this kind of thing, and maybe we build a bit of a layer on top to avoid needing to define properties that aren't relevant for this kind of widget/area. Most of the parts of the Widget API are still relevant though like declaring
modules
for use with Dashboard Sharing, so it doesn't seem that a new API of sorts would be warranted.Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Dashboard*App
andDashboardEntryPoint
components and moved to module-registered locationsNote: the example in the description is not a requirement but could be used as an part of an initial implementation. It could be worth exploring what a proper API for this would look like, although there would likely be substantial overlap with features already provided by the widgets API.
Implementation Brief
assets/js/googlesitekit/modules/index.js
registerModule
to list two new optional components:MainRootComponent
andEntityRootComponent
assets/js/googlesitekit/modules/datastore/modules.js
registerModule
action -MainRootComponent
andEntityRootComponent
and add them to thesettings
variableassets/js/components/ModuleRootComponents.js
dashboardType
, it should be string enum, holding eithermain
orentity
valuegetModules
selector fromCORE_MODULES
store to fetch all modulesmodules
and filter for the ones that have propertyconnected
and depending on thedashboardType
prop, if it ismain
then filter in for the modules that haveMainRootComponent
property, otherwise filter forEntityRootComponent
and save the results in a variable, sayfilteredModules
for examplefilteredModules
is empty return earlyfilteredModules
and render theirMainRootComponent
orEntityRootComponent
depending on the value ofdashboardType
propassets/js/components/DashboardEntryPoint.js
ModuleRootComponents
together with eitherModuleSetup
orDashboardMainApp
depending on the current value ofsetupModuleSlug
assets/js/components/DashboardEntityApp.js
ModuleRootComponents
component afterScrollEffect
component. Pass thedashboardType
prop with value ofentity
.DashboardMainApp
andsyncGoogleTagSettings
call with it'suseMount
hook fromDashboardEntryPoint
into a separate componentassets/js/modules/analytics-4/components/MainRootComponent.js
, which only contain hooks, and return null in the renderer.MainRootComponent
component inassets/js/modules/analytics-4/index.js
to theregisterModule
function asMainRootComponent
property.Test Coverage
assets/js/googlesitekit/modules/datastore/modules.test.js
to verify new components. You can useaccepts settings components for the module
anddefaults settings components to
nullif not provided
as starting pointQA Brief
Changelog entry
The text was updated successfully, but these errors were encountered: