Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

Showcase/29 jan initial #27

Merged
merged 10 commits into from Jan 28, 2021
Merged

Showcase/29 jan initial #27

merged 10 commits into from Jan 28, 2021

Conversation

zackads
Copy link
Contributor

@zackads zackads commented Jan 28, 2021

Stand by for second PR with the "change" I propose for the demo

Prettier/ESLint was trimming whitespace, resulting
in incorrect rendering
Add GOV.UK classes to <html> and <body> tags so page layout is correct.

Also raised issue #25 to do this properly after the show and tell.
Only page content now need go in each individual page file
This causes the Header to display the crown crest twice (as both SVG and
PNG), so the PNG is commented out. Issue raised:
#26
Copy link
Contributor

@matthew-a-carr matthew-a-carr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a few changes to make :)

src/pages/_document.tsx Show resolved Hide resolved
render(): React.ReactElement {
return (
// TODO: https://github.com/madetech/mca-beacons-webapp/issues/25
<Html className={"govuk-template "}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this class name required here? If it's not adding anything I would say remove the class name...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @matthew-a-carr, I should have added why these classes are important. Without them the layout and background colour of the footer is off (note the whitespace at the bottom):

image

Both are from the GOV.UK Page template.

I haven't had the time to look into including the GOV.UK page template to html and body etc. thoroughly as I wanted to get something set up for the showcase. I raised issue #25 to revisit this (including the console error) tomorrow morning / next week.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, happy in that case. Will just need to raise in planning the issues that we're tracking here as they aren't currently captured on the backlog in Trello and we will need to account at least some time to them for next sprint

src/pages/_document.tsx Show resolved Hide resolved
@matthew-a-carr
Copy link
Contributor

@zackads Also, I am getting this in the console:
image

It looks like the suggestion is to set the title element in the _app.tsx file instead, see: vercel/next.js#4596. Example merge request from another project: https://github.com/primer/components/pull/352/files

Copy link
Contributor

@matthew-a-carr matthew-a-carr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, following conversation regarding issues being captured in GitHub

render(): React.ReactElement {
return (
// TODO: https://github.com/madetech/mca-beacons-webapp/issues/25
<Html className={"govuk-template "}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, happy in that case. Will just need to raise in planning the issues that we're tracking here as they aren't currently captured on the backlog in Trello and we will need to account at least some time to them for next sprint

@zackads zackads merged commit 0c6a84a into main Jan 28, 2021
@zackads zackads deleted the showcase/29_jan-initial branch January 28, 2021 16:48
@zackads zackads restored the showcase/29_jan-initial branch January 28, 2021 16:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants