Add a content area to the Header in the Unified Dashboard for Entity Info and notifications

[tofumatt]

Feature Description

There should be a section in the header of the dashboard that displays content in the view between the header and the pill links, see:

The logic to display this content is covered elsewhere; this issue is just above creating the component that can be used to style either the Entity information (shown above) or the Notifications that appear in this area when any are present.

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• A new prop subHeader should be added to the Header component. It should be used to place arbitrary content in the header area highlighted in the screenshot. It should appear below the <header> tag in the Header component. (https://github.com/google/site-kit-wp/blob/5f12f3b3b59b1517bc2a0efa4d427120de2e4239/assets/js/components/Header.js#L41-L71)
• This prop should be used by the components in #4146 and #4152 to wrap their output when inside the <Header> component
• When the subHeader prop is passed components/eg. a non-null value, the output should match the styles in the Figma mockup: https://www.figma.com/file/VILRieNSLg2DOVyEgFBkXe/%5BMVP%5D-Generic-Dashboard?node-id=459%3A2117 (note how the bottom border/shadow of the header becomes the border/shadow of the subheader instead when there is one, i.e. it's almost like the header expands). There should be no inner padding though as that should be defined by whatever is rendered within the container.
• The component should NOT be sticky, unlike the rest of the header. When the user scrolls down the page, this section of the header should disappear and not follow the user down the page.
• The BannerNotifications component from #4152 should be rendered within the new sub-header container, but only if on the "main dashboard" (i.e. not if on the "entity dashboard").

Implementation Brief

Integration in Header

• Update Header to accept a new subHeader prop which should take an optional element
  • If provided, this element should be rendered in a new grid inner (row), and full-width grid cell below the existing elements wrapping the main menu (i.e. as a child of mdc-layout-grid)
• Update stories/header.stories.js to add a new story for showing the header with a dummy component
• Add subHeader={<BannerNotifications />} to the DashboardMainApp's Header component (https://github.com/google/site-kit-wp/blob/94ab93a3bf301b165aebfdb0b4df75a815ffd96b/assets/js/components/DashboardMainApp.js#L42) to output the notifications on the Main Dashboard.

Test Coverage

• Add a Storybook story to show what the header looks like with "Empty" or "Dummy" content—eg the padding and extra border dividing the header from the subheader should appear, with another border between the subheader and navigation

QA Brief

• Enable the unifiedDashboard feature flag.
• Check if the a notification is rendered below the header at the following URL: /wp-admin/admin.php?page=googlesitekit-dashboard&notification=authentication_success
• There should be no visible lines separating the header and the sub header content.
• Scroll down the page, the header should be sticky.

Changelog entry

• Add a generic content area for the Header in the Unified Dashboard.

[kuasha420]

I have made some observation while working on the IB according to Acceptance Criteria.

• From my chat with @aaemnnosttv, we thought Slot/Fill could be used for this, and we can create an abstraction over it to roughly achieve what we are after. In that case,
  • We can use normal declarative approach that is commonly used in react to show and hide stuff from the area. ie. {showNotification && <Fill priority={2}>{content}</Fill>}.
  • In that case we can skip the whole imperative approach with a function to add and remove components from the header.
  • I have a rough IB Draft to achieve that using custom abstraction on top of the aforementioned Slot/Fill and I will polish it up and send it for review if this approach is sufficient and we really don't need the imperative API as per the AC.
• Otherwise, we may need to consider different approach.

Looking for feedback.

CC @tofumatt @aaemnnosttv

[tofumatt]

Totally, that's actually a way better approach and I think will totally work for what we want to do. I think I even wanted to propose it in the ACs originally, but for some reason I guess my brain went with the more imperative approach.

But Slot/Fill is way nicer AND less code! 🙂

I've updated the ACs, let's go with that approach. If any other ACs related to this issue need updating I'll do that too 👍🏻

[tofumatt]

After a discussion with Eugene, Felix, and Evan, we actually realised that we don't need the slot/fill implementation here, at least for now, and can get away with this component rendering a few, specific things according to the design:

1. On the Entity Dashboard only, the current page name, URL, and a "Back to Dashboard" link.
2. The notifications area on both the Main and Entity dashboard.

I think originally we had planned for this to be a more generic content are that would be used by other things, but we couldn't remember what for and I think that need has gone 😓

So I've reworked the ACs as this is now a much less complicated component/set of ACs

Sorry @kuasha420 for making you think on theses ACs and write up that solid IB for slot/fill, though I have copied it to a new issue (#4212) in case we want to revisit slot/fill for the header (or any other section of the plugin) in the future.

I've moved this back to IB now that there are new, simpler ACs (again!) for the issue. Hopefully this is the last revision we make to the ACs! 😅

[felixarntz]

@tofumatt I think the ACs could even be further simplified, as there's already dedicated issues for the two pieces of content (#4146 and #4152) to conditionally render in here. I think the last two bullet points could be removed from the ACs, as those parts are to be implemented in those issues. I'd say in this issue we should focus only on getting the area ready, and it might be a good idea to include a Storybook story with some demo content for it, just to see what it looks like with "any" content inside and to ensure the styling makes sense.

cc @kuasha420

[tofumatt]

Hmmm, we shouldn't be rendering the <Subheader> area at all if there are no notifications/no entity info to show though. It requires another border to be drawn between the Header and the Navigation bar.

I suppose if it's currently only possible for one piece of content to appear in the header at a time—given notifications won't appear on the Entity Dashobard… maybe this should just be thought of as the "wrapper component" with no logic. I think that makes sense, but it's just a re-framing of the issue.

I've updated the ACs again, but I've assigned you to have a look at them to make sure they make sense @felixarntz. If they're good feel free to move this over to IB 👍🏻

[felixarntz]

@tofumatt Ah that's a good point that we need an actual container. I wonder though, we might still be able to have the Subheader not have any visual effects when rendered empty - the border is technically a shadow I believe from looking at the Figma files, and the shadow would be on the headerbar and the tab/pill bar, so not on the Subheader itself. We don't need any padding within it for example (the padding would be handled in the respective inner components), so I'd think if its inner content is empty / null, it wouldn't show at all (while technically still being in the DOM). I'd be okay with that.

Regarding the Figma design itself, it looks like that new shadow above was recently added - but the Figma file also has some note about advising not to do that. I left a comment asking for clarification.

[felixarntz]

In order to unblock #4152 (which is already in Execution Backlog), I've added one more bullet point here, essentially reversing the order on which these two items should be worked on.

[felixarntz]

@tofumatt Any particular reason why you removed the one extra bullet point I had added to the ACs (see https://github.com/google/site-kit-wp/issues/4050#issuecomment-941742576)? I assume it was by accident, so I've added it back for now. Let me know your thoughts - if we keep it, we should also mention that in one extra point in the IB (otherwise the IB looks good).

[felixarntz]

@tofumatt Actually, one more question about the ACs:

This prop should be used by the components in #4146 and #4152 to wrap their output when inside the <Header> component

How do you envision that to work? The Notifications component e.g. would be rendered within the Header, how can it pass a prop to the Header?

[felixarntz]

@tofumatt Last but not least, I've updated the ACs one more time: The Figma link you had pointed to before was outdated, indeed there should not be a border between the header and subheader, as Jenna clarified on that previous Figma file in response to my comment. There should only be one border/shadow at the bottom of the header or subheader, as originally defined.

[tofumatt]

@tofumatt Any particular reason why you removed the one extra bullet point I had added to the ACs (see #4050 (comment))? I assume it was by accident, so I've added it back for now. Let me know your thoughts - if we keep it, we should also mention that in one extra point in the IB (otherwise the IB looks good).

Oh, sorry, I think I was thinking that the BannerNotifications component would add themselves into the subHeader once available, but I suppose since the issue is already in the backlog that won't work.

In that case I'll add it to the IB here, that's fine 👍🏻

How do you envision that to work? The Notifications component e.g. would be rendered within the Header, how can it pass a prop to the Header?

I don't think it would need to be—I think the notifications component (in this case, BannerNotifications from #4152) could be supplied to the subHeader prop whenever needed (eg in the DashboardMainApp component, right here: https://github.com/google/site-kit-wp/blob/94ab93a3bf301b165aebfdb0b4df75a815ffd96b/assets/js/components/DashboardMainApp.js#L42).

We can supply multiple components into the subHeader when needed by wrapping in a <Fragment>, though I think for now the Main Dashboard just needs notifications and the Entity Dashboard just needs the component in #4146.

Does that sound good?

[felixarntz]

@tofumatt Ahh, thanks for clarifying. I think I was confused by the wording: "This prop should be used by the components in #4146 and #4152 to wrap their output when inside the <Header> component" says it should be used by the components, but the way I understand you now, which makes sense to me, the prop should be used by any component to pass the inner components :)

Now that that's clarified, that makes sense. Note though that current approach still wouldn't ensure that the sub-header only renders if there is actually something in there. That is because e.g. the BannerNotifications component contains logic inside and based on the logic may return null. In that case, there would still be an empty sub-header. Which again, may not be a dealbreaker if we can ensure that empty container doesn't have a visual impact, but I want to make sure you've considered that.

[tofumatt]

In that case, there would still be an empty sub-header. Which again, may not be a dealbreaker if we can ensure that empty container doesn't have a visual impact, but I want to make sure you've considered that.

Yes, especially now that there is only one border between the nav bar and the entire header, I think this will be fine. Essentially, if there is zero height to whatever is rendered inside the subHeader prop then the "subheader" area should be empty.

So I think this approach should do fine and this IB should cover it, but let me know if you have any other questions about this approach. 👍🏻

[felixarntz]

Sounds good - IB ✅

[asvinb]

@tofumatt @felixarntz @kuasha420 A few things I noticed

• The IB mentions having the subHeader prop displayed within mdc-layout-grid. I think we should not do that, but instead leave the implementation to the rendered component. For e.g the Notification component already uses mdc-* class names. So I would suggest we just have a full width wrapper instead of having a grid layout.
• The box-shadow differs on small and large screens. I suggest we keep them consistent and since we are not really focusing on the designs in Figma, I suggest we use the shadow mixin we already have.
• We should cater for when the header is sticky to show the box-shadow even though when we have a sub header because the latter is not sticky.

I have a PR created for the above points.

Let me know what you think.

[felixarntz]

@asvinb Great points. Your point about a full-width container is also in the ACs - it says there that it shouldn't have any inner padding, which I basically meant to express with that it should be full-width and not assume/restrict anything about the content within :)

[wpdarren]

@felixarntz @asvinb @tofumatt Looking at the initial ticket description and AC's, the QAB doesn't appear to be accurate. Please could you clarify if it is, and if not, any additional info would be appreciated.

[asvinb]

@wpdarren The QAB says going to the following URL: /wp-admin/admin.php?page=googlesitekit-dashboard&notification=authentication_success. In doing so, the BannerNotifications component will be passed to the Header component via the subHeader prop. It's a way to test the latter. Then it should match the designs in Figma, meaning there should no be any borders between the header and the sub header (BannerNotifications in this case), and then only the header is sticky upon scrolling.

Let me know if that clears things up.

[wpdarren]

QA Update: ✅

Verified:

• There are no visible lines separating the header and the sub header content.

• When scrolling down the page, the header is sticky.

• Confirmed the same behaviour for small screen sizes.