Skip to content
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

Add entity context component to render in the header's “extra content area” when on the Entity Dashboard #4146

Closed
tofumatt opened this issue Sep 28, 2021 · 12 comments
Labels
P0 High priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature

Comments

@tofumatt
Copy link
Collaborator

tofumatt commented Sep 28, 2021

Feature Description

There should be info about an entity when on the Entity Dashboard.

The full version (when the page has no yet scrolled) should look like this:

Screenshot 2021-09-28 at 16 03 44

It should be inserted into the header "content" area created in #4050.


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

Acceptance criteria

Implementation Brief

  • Create assets/js/components/EntityHeaderBanner.js which exports the EntityHeaderBanner functional component.
  • Check if the component is being rendered on the main dashboard by checking if the view context is VIEW_CONTEXT_DASHBOARD. If the context isn't VIEW_CONTEXT_DASHBOARD, the component should return null
  • The "Return to dashboard" label can be hard coded in the component. The 'Detailed page stats for: "$page_title"' should use sprintf( __( '<strong>Detailed page stats for:</strong> "%s"' ), pageTitle ) to ensure the entire string is properly localised.
    • The "Return to dashboard" label should appear visually on tablet screens and above using CSS. (using the $bp-tablet breakpoint). But ensure that the text is placed off-screen and not set to display: none on mobile viewports, so the text is still shown to assistive technologies like screenreaders.
  • To get the entity URL and label, query the site data store via the respective selectors:
    • getCurrentEntityURL
    • getCurrentEntityTitle
  • Apart from hiding the "Return to dashboard" label on small screens via CSS, no other styles should be added.
  • Once Add a content area to the Header in the Unified Dashboard for Entity Info and notifications #4050 has been merged, add EntityHeaderBanner within the container where the subHeader prop is displayed.
  • Update stories for "Header" to include a story for EntityHeaderBanner.

Test Coverage

  • No new tests to be added.

QA Brief

Open the storybook story and confirm that it matches the figma designs (in AC):
https://google.github.io/site-kit-wp/storybook/develop/?path=/story/components-header--header-with-sub-header-entity-banner

Enable unifiedDashboard
Open:
/wp-admin/admin.php?page=googlesitekit-dashboard&notification=authentication_success&permaLink=http%3A%2F%2Fsitekit.10uplabs.com%2Fsample-page%2F where the permaLink matches a page that exists on your site, you can find a page via:
/wp-admin/admin.php?page=googlesitekit-dashboard&notification=authentication_success url search in the header component. The notification notification=authentication_success is necessary to render the subHeader.

Verify it matches the figma and press the back button to return to dashboard.

Changelog entry

  • Add entity header content area to the Unified Dashboard.
@tofumatt
Copy link
Collaborator Author

Looks good; I made a tweak to one of the strings to ensure it's localised properly.

Also, the first reference to mobile CSS said to show the labels only on mobile, but later said to hide the "Back to Dashboard" label on mobile. The latter is correct (and what's in the Figma mockups), so I assume the first was just a typo and I changed it 🙂

IB ✅

@tofumatt tofumatt removed their assignment Oct 20, 2021
@fhollis fhollis removed this from the Sprint 61 milestone Oct 26, 2021
@ivankruchkoff ivankruchkoff self-assigned this Nov 5, 2021
@ivankruchkoff
Copy link
Contributor

A few questions here @tofumatt (with draft PR in #4384)

After resolving the above, I'll need some CSS assistance, as it currently looks like:
Screen Shot 2021-11-19 at 7 55 18 PM

@eclarke1 eclarke1 added the Rollover Issues which role over to the next sprint label Nov 22, 2021
@tofumatt
Copy link
Collaborator Author

tofumatt commented Nov 22, 2021

"Return to dashboard" label - that's not shown in the figma, can you elaborate on where that should be?

Indeed, looks like that's gone. It would be good to have that as a visually hidden bit of text for the "back" arrow link (to the left of Detailed page stats for: "x", just for screen-reader users. But looks like that isn't anywhere visually anymore.

'<strong>Detailed page stats for:</strong> "%s"' potentially introduces xss, I've gone with this approach: https://github.com/google/site-kit-wp/pull/4384/files#diff-06c928c9cf11b408f20de7e5abe65ca4adcbd24c6a3a695ffc9dd7df039cde34R60-R65

The approach there separates the string which can cause issues for some translations. I didn't mention it in the IB Review but we use createInterpolateElement to translate strings with tags (and even React elements):

{ createInterpolateElement(
. So that answers this question and the next 🙂

For QA, how would we set up a test so that getCurrentEntityURL and getCurrentEntityTitle are set without going down the QA:eng route or just verifying via the story ?path=/story/components-header--header-with-sub-header-entity-banner

Verifying the story is probably fine, but those are set when the permaLink query param is set, eg: something like http://sitekit.10uplabs.com/wp-admin/admin.php?page=googlesitekit-dashboard&permaLink=https%3A%2F%2Fwww.elasticpress.io%2Fblog%2F

Let me know if that answers anything or if I should clarify something further 👍🏻

@tofumatt tofumatt removed their assignment Nov 22, 2021
@ivankruchkoff
Copy link
Contributor

Thanks for the update @tofumatt

I've changed to use createInterpolateElement in 506357c and also changed the viewcontext from VIEW_CONTEXT_DASHBOARD to VIEW_CONTEXT_PAGE_DASHBOARD which is the value on /wp-admin/admin.php?page=googlesitekit-dashboard&permaLink=http%3A%2F%2Fsitekit.10uplabs.com%2Fsample-page%2F

For QA, that scenario you linked won't suffice, as with a permaLink property we still don't have a subHeader property to render the EntityHeaderBanner here:

<EntityHeaderBanner />

@ivankruchkoff
Copy link
Contributor

Separate comment to call out :

After resolving the above, I'll need some CSS assistance, as it currently looks like: Screen Shot 2021-11-19 at 7 55 18 PM

To get the component to load, you can open a URL like /wp-admin/admin.php?page=googlesitekit-dashboard&permaLink=https%3A%2F%2Fwww.elasticpress.io%2Fblog%2F replacing the permaLink with a valid link on your site and either setting a subHeader prop in Header or making a temporary tweak to subHeader || ( in:

@asvinb
Copy link
Collaborator

asvinb commented Nov 23, 2021

@tofumatt @ivankruchkoff Just FYI, the "Return to dashboard" label is visible only on large screens and not on small screens:
https://www.figma.com/file/VILRieNSLg2DOVyEgFBkXe/%5BMVP%5D-Unified-Dashboard?node-id=2%3A6378

@ivankruchkoff
Copy link
Contributor

Over for CR:
Screen Shot 2021-11-23 at 7 47 55 AM

Mobile:
Screen Shot 2021-11-23 at 7 45 53 AM

and from storybook:

Screen Shot 2021-11-23 at 7 55 32 AM

Mobile:

Screen Shot 2021-11-23 at 7 55 21 AM

@ivankruchkoff
Copy link
Contributor

@tofumatt over to you for CR.

We didn't have a subHeader prop set on the DashboardEntityApp that is rendered for:
/wp-admin/admin.php?page=googlesitekit-dashboard&notification=authentication_success&permaLink=http%3A%2F%2Fsitekit.10uplabs.com%2Fsample-page%2F

I added 3e473ca to render the same as:

<Header subHeader={ <BannerNotifications /> } showNavigation>
<EntitySearchInput />
<DateRangeSelector />
<HelpMenu />
</Header>

@tofumatt tofumatt removed their assignment Nov 25, 2021
@wpdarren wpdarren self-assigned this Nov 26, 2021
@wpdarren
Copy link
Collaborator

QA Update: ⚠️

@ivankruchkoff just two observations. Are these expected?

  • On Storybook and test site, the link color is different (#1a73e8) from the Figma (#2998e8)
  • On test site, the font weight is 600, on Figma it is 700. for the Detailed page stats for text.

@ivankruchkoff
Copy link
Contributor

@wpdarren in the IB we have: Apart from hiding the "Return to dashboard" label on small screens via CSS, no other styles should be added., however I know we introduced some CSS changes, so perhaps @asvinb / @tofumatt can comment on it.

@asvinb
Copy link
Collaborator

asvinb commented Nov 29, 2021

QA Update: ⚠️

@ivankruchkoff just two observations. Are these expected?

  • On Storybook and test site, the link color is different (#1a73e8) from the Figma (#2998e8)
  • On test site, the font weight is 600, on Figma it is 700. for the Detailed page stats for text.

@wpdarren

  • We are using the existing link color from the codebase. Let's stick with #1a73e8 to be consistent with the current styles.
  • We are using the strong tag for the Detailed page stats for and WordPress actually sets the font-weight to 600. However we are not loading the 600 font-weight for Google Sans as per screenshot: image and thus it falls back to font-weight: bold which is actually 700. So it's fine. You will notice that if you change the font-weight from 600 to 700, you won't see any difference.

@wpdarren
Copy link
Collaborator

QA Update: ✅

  • A new component EntityHeaderBanner is displayed, containing the title and URL of the current entity.
  • The new component renders in the inner content from that white "sub-header" area that shows between the header bar and the pills/tabs navigation bar.
  • The component itself does not receive any of the background-color, border, shadow etc.
  • Storybook is added for the EntityHeaderBanner within the overall Header as well.
  • Checked the desktop and mobile version compared with Figma designs.

Desktop

image

Mobile

image

@wpdarren wpdarren removed their assignment Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants