Closed felixarntz closed 3 years ago
@felixarntz – overall the IB looks good to me. A few questions/thoughts:
- Introduce
DashboardFooter
component which renders aHelpLink
within adiv
that causes it to render right-aligned.- Introduce
DashboardDetailsFooter
component which renders aHelpLink
within adiv
that causes it to render right-aligned.
Why not use DashboardFooter
for the Footer
prop in both cases rather than introduce a duplicate component?
- Add a
featureFlags.widgets.dashboard.enabled
rendering conditional abovediv.googlesitekit-module-page
, below rendering theHeader
, to separate the new Widgets API-based rendering from the current/legacy rendering entirely.
We should probably refer to the feature flag condition a bit more abstractly (or in terms of how it will be) since #2452 will likely be merged before this issue is picked up.
One thing that comes to mind here regarding the transition to the Widgets API is that it might be a bit easier to transition by wrapping the legacy layout in a single widget area + full-width widget. Then the feature flag would simply influence which context is provided to the area (e.g. dashboard
or legacyDashboard
). That way we wouldn't need to have completely separate wrapping components and we could introduce the Widgets API a bit more gradually rather than all at once since the main risk is more around the new widgets rather than the API itself.
@aaemnnosttv
Why not use
DashboardFooter
for theFooter
prop in both cases rather than introduce a duplicate component?
We could, but I think since these two are different area-specific components and since it is not set in stone that those would necessarily render the same thing, I think it makes sense to have them separate.
We should probably refer to the feature flag condition a bit more abstractly (or in terms of how it will be) since #2452 will likely be merged before this issue is picked up.
Good point, updated.
One thing that comes to mind here regarding the transition to the Widgets API is that it might be a bit easier to transition by wrapping the legacy layout in a single widget area + full-width widget. Then the feature flag would simply influence which context is provided to the area (e.g.
dashboard
orlegacyDashboard
). That way we wouldn't need to have completely separate wrapping components and we could introduce the Widgets API a bit more gradually rather than all at once since the main risk is more around the new widgets rather than the API itself.
I think the idea is worth considering overall, but the way the legacy components are implemented I think it's easier to keep them separate as is. Putting the legacy components into a separate widget would probably be more work than is needed here, and it could result in unexpected breakage. I'd prefer to touch the old components as little as possible and just wait for them to be replaced/removed.
It shouldn't be too much of a problem to wait until the Widgets API-based dashboard has been rolled out to production for the Widgets API to be used. With all the widgets Site Kit will have it will also be a better starting point for 3P developers, since they can then look at those widgets for reference.
@felixarntz IB LGTM – the only thing that was a bit confusing was understanding the changes to DashboardDetailsApp
and how DashboardDetailsEntityView
and DashboardDetailsEntityNotFoundView
are being replaced (merged into) the new DashboardDetailsHeader
since this isn't mentioned until further down. I'll make a note of this above but otherwise great!
IB ✅
@aaemnnosttv I am seeing quite a few differences but not in layout. E.g. on the Site Kit dashboard, on development build the Search funnel says Unique Visitors From Search
, where as on production build it is saying Unique Visitors
. I know the ticket suggests this is just layout, but would like to double check.
@wpdarren I noticed a few differences between the widget-based and legacy dashboard when doing my review but not as a result of this issue. Please make a note about all of the differences you see and we can see which ones need issues or which are intentional changes.
@aaemnnosttv here are the differences I've found between production and development build for the Site Kit Dashboard and Detailed Page Stats page. The layout looks fine, so this can be passed but would appreciate it if you could look through the differences and let me know if these need a new ticket or not.
There are a number of differences between the production and development build. I've listed them all here, even though the change in this ticket might not necessarily be the cause. New tickets can be created.
Production build
Development build
Production Build
Development Build
Production Build
Development Build
Popularity
whereas on Development build it is Top Queries
related to search console pages. Production Build
Development Build
On the Detailed Page Stats page. Under Page speed. The list of recommendations are ordered differently. (see screenshots above)
Under Search Funnel. The unique visitors title is different. On Production build it is Unique Visitors
but Development build says Unique Visitors from Search
Production build
Development build
The comparison above was for a site with analytics, etc. I did the same with a site that installed Site Kit for the first time.
Use goals to measure success
but the development build didn't Popularity
section but didn't in Production BuildThanks for the detailed report @wpdarren! I think these are probably unrelated but some of them are probably worth opening issues for. I'll assign this to @felixarntz to confirm.
@felixarntz wondered what your thoughts on my review of the differences? Are these related to this ticket or would you like me to create a new ticket? I know the ticket mentions more the padding/nesting which does not appear to be an issue.
@wpdarren Thanks for the ping, sorry for the delay. Some feedback:
Overall observation, I think you accidentally mislabelled some of the screenshots between develop and production build? At least under 2, 3, 4, and 6, the screenshots are each for the respective other build.
Note that, at this point, and for any future reviews, using a development build vs production build should no longer matter. Feature flags are now solely controlled via the 1.4.0 version of the Site Kit tester plugin; so going forward you should be able to always use a production build, and then compare state with the respective feature flag turned on or off.
@felixarntz thank you for confirming these for me. I will give this another run through based what you've said about the 1.4.0 tester plugin and then move this forward. To confirm 3, and 7 you have/will create tickets for these.
Verified:
On the Site Kit Dashboard: There are no extra padding or any visual disparities. Both include the "Site Overview" heading, and they should both include the "Need help?" link in the footer.
On the Single URL dashboard: There are no extra padding or any visual disparities. Both include the "Site Overview" heading, and they should both include the "Need help?" link in the footer.
Currently there is some extra padding and various nested HTML elements around the new Widgets API-based dashboard which are mostly there because the new
WidgetContextRenderer
was embedded into the existing legacy DOM structure. We should fix that so that theWidgetContextRenderer
practically can take care of rendering the entire page itself.Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
DashboardApp
) and the single URL dashboard (seeDashboardDetailsApp
) should have the same visual padding from the main page border as its current/legacy version does.DashboardApp
) and the single URL dashboard (seeDashboardDetailsApp
) should work like it would if it was the "only thing on the page", i.e. it should have its own (duplicate) header and (duplicate) "Need help" link in the footer.WidgetContextRenderer
component should be able to take care of rendering basically the full page content (except the overall Site Kit header bar):className
prop so that thediv.googlesitekit-widget-context
element can have additional class names.Header
prop for a component to render as the overall header for the widget areas within the context.Footer
prop for a component to render as the overall footer for the widget areas within the context.Implementation Brief
className
prop toWidgetContextRenderer
(withPropTypes.string
). If passed, combine it with the automatically addedgooglesitekit-widget-context
class so that the outer div has all of those classes.Header
prop toWidgetContextRenderer
(withPropTypes.elementType
). If passed, it should be rendered within thediv.googlesitekit-widget-context
, but before all widget areas, and it should be wrapped within a<Grid><Row><Cell size={ 12 }>
so that it's correctly indented.Footer
prop toWidgetContextRenderer
(withPropTypes.elementType
). If passed, it should be rendered within thediv.googlesitekit-widget-context
, but after all widget areas, and it should be wrapped within a<Grid><Row><Cell size={ 12 }>
so that it's correctly indented.DashboardHeader
component which renders thePageHeader
component with the same props that it's rendered with above the legacy widgets inDashboardApp
.DashboardFooter
component which renders aHelpLink
within adiv
that causes it to render right-aligned.DashboardApp
component:PageHeader
with rendering the newDashboardHeader
.widgets.dashboard
feature flag is enabled abovediv.googlesitekit-module-page
, below rendering the notifications, to separate the new Widgets API-based rendering from the current/legacy rendering entirely.Cell
that wrapsWidgetContextRenderer
.className
prop toWidgetContextRenderer
with valuegooglesitekit-module-page googlesitekit-dashboard
, to keep these classes applying to the new Widgets API-based rendering as well.Header
prop toWidgetContextRenderer
withDashboardHeader
.Footer
prop toWidgetContextRenderer
withDashboardFooter
.DashboardDetailsHeader
component which renders theLink
andPageHeader
component fromDashboardDetailsApp
, and also conditionally theDashboardDetailsEntityHeaderContainer
from eitherDashboardDetailsEntityView
orDashboardDetailsEntityNotFoundView
, depending on whether there is a current entity.Note:
DashboardDetailsEntityView
andDashboardDetailsEntityNotFoundView
will be reimplemented viaDashboardDetailsHeader
and will be removed (see below)DashboardDetailsFooter
component which renders aHelpLink
within adiv
that causes it to render right-aligned.DashboardDetailsApp
component:Link
andPageHeader
andDashboardDetailsEntityView
/DashboardDetailsEntityNotFoundView
with rendering theDashboardDetailsHeader
and, if there is a current entity, theDashboardDetailsModules
component.widgets.pageDashboard
feature flag is enabled abovediv.googlesitekit-module-page
, below rendering theHeader
, to separate the new Widgets API-based rendering from the current/legacy rendering entirely.WidgetContextRenderer
.className
prop toWidgetContextRenderer
with value "googlesitekit-module-page googlesitekit-dashboard-single-url`, to keep these classes applying to the new Widgets API-based rendering as well.DashboardDetailsHeader
asHeader
prop andDashboardDetailsFooter
asFooter
prop.slug="pageDashboard"
. Otherwise, passslug="pageDashboardNotFound"
to ensure no widget areas are rendered there. (Technically, this relies on another (non-existing) widget area, so we could at some point leverage that to add e.g. a search widget to the page or help the user in another way.)DashboardDetailsEntityView
andDashboardDetailsEntityNotFoundView
components entirely.Dashboard*
components withLegacy*
, e.g.LegacyDashboardModules
,LegacyDashboardDetailsModules
etc.Test Coverage
Visual Regression Changes
QA Brief
Changelog entry