Fix/update cms component with meta data

[luke-denton-aligent]

Description

This PR updates the cms.js component to make use of data.cmsPage.title and data.cmsPage.content_heading, that otherwise seems to be ignored.

I have placed the <Title> and <h1> in this location so that it doesn't affect the home page. I am by no means saying this is the perfect place to put it, I'm very open to suggestions of where to move these elements, but I think they should be included somewhere.
I have attached a couple of screenshots, showing an "about-us" page with and without this update. In the "before" screenshot, we can see how hard it is to determine which page we're on, especially if we have multiple tabs open on the storefront.

The classname given to the <h1> is also a bit of a guess, so I'm open to updating this to better suit.

Related Issue

Closes #2158.

Acceptance

Verification Stakeholders

Specification

Verification Steps

1. Create a new page
2. Give the new page a content heading, and some content
3. Navigate to the URL slug of the new page using a PWA instance
4. Verify that the page title updates to reflect the name of the page that you're on, and that the content heading that was entered into admin is also displaying

Screenshots / Screen Captures (if appropriate)

Before Update

After Update

[PWAStudioBot]

	Fails
🚫	A version label is required. A maintainer must add one.

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

Generated by 🚫 dangerJS against c481fc4

[jimbo]

For readability, we avoid multiline JSX expressions, since they defeat the readability advantages of JSX. Try extracting this to a const.

const { content_heading, title } = data.cmsPage

// conditionally define an element
// note that the alternative is null, not undefined
const headingElement = content_heading ? (
  <h1>
    {content_heading}
  </h1>
) : null

// more readable without the conditional expression in brackets
const content = foo ? (
  <Fragment>
    <Title>{title}</Title>
    {headingElement}
  </Fragment>
) : (
  <CategoryList />
)

/* ... */

[luke-denton-aligent]

Thanks Jimbo, I would have to disagree with this approach being better for readability, I'm very much disliking having to work with the code that's been broken up like this when molding PWA Studio to my needs. In saying that, I'll happily make the changes to match Magento's standards

[jimbo]

This works, but we prefer <Fragment>. Be sure to import it at the top.

import React, { Fragment } from "react"

[luke-denton-aligent]

Happy to make this update, but would just like to point out that the only reason I used the shorthand here is because I thought Magento were preferring it, as it's used in many files in PWA Studio. A simple Find In Files in the codebase will find 8 other places that the shorthand is used, including the very file that I've edited

[brendanfalkowski]

FWIW, I started out liking React.Fragment but found a typo broke the whole build with a non-helpful error. We ended up switcing everything to Fragment destructured because that error is dev-traceable.

And we specifically avoided <> because at least one major editor (Atom) doesn't understand how to auto-close tags with no node-name, which is frustrating.

[luke-denton-aligent]

Sure thing! I totally agree, I much prefer seeing <Fragment> rather than the magic <>. I just wanted to point out the use of the shorthand because I'm being told that the full tag is preferred, when in the actual component I'm editing it's clearly not being used. If the decision has been made since this code was written, that's totally fine, but just need to make sure we're aware of these sort of things when making statements like X is preferred over Y

[jimbo]

We don't have use global classnames in our components, and we don't target them with global CSS. If you'd like to render this element with some styles, consider defining a .heading classname in cms.css.

If styles are not being imported in this component yet, have a look at another component in the codebase to see how to import them, merge the classes, and apply them to elements. Something like the following:

import defaultClasses from "./cms.css"

const CMSPage = props => {
  const classes = mergeClasses(defaultClasses, props.classes)

  return (
    <div className={classes.root} />
  )
}

[luke-denton-aligent]

No worries, can make this update

[luke-denton-aligent]

Thanks for the review @jimbo, I've made the requested changes now and this is ready for a second review. Thanks!

[luke-denton-aligent]

@jimbo Just wanted to bump this, as I've implemented the requested changes

[dpatil-magento]

@luke-denton-aligent - There is a prettier failure (packages/venia-ui/lib/RootComponents/CMS/cms.js) and home page does not load at https://pr-2159.pwa-venia.com/ ( tried on my local as well watch:venia find below console log attached), pls check.
cc @jimbo

[luke-denton-aligent]

Hi @dpatil-magento, thanks for the feedback.

I've just loaded up the PR URL, and while I'm also seeing as red banner, I don't see any errors in the console.

It would be really great if the screenshot of devtools wasn't cut off - we can't currently see where that reference error is being thrown

[luke-denton-aligent]

Ah, thanks!

[tjwiebell]

@luke-denton-aligent - Those PR instances are in production mode that have terser suppressing all console logs, so @dpatil-magento must have grabbed that locally, sorry for the confusion.

This was just a tiny import missing and needing yarn prettier ran, so I went ahead and did that for you in 20cca3d. We should probably consider adding a bot that just commits prettier changes instead of holding up PRs, but right now its manual.