-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
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
There was a problem hiding this 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 :)
render(): React.ReactElement { | ||
return ( | ||
// TODO: https://github.com/madetech/mca-beacons-webapp/issues/25 | ||
<Html className={"govuk-template "}> |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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):
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.
There was a problem hiding this comment.
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 Also, I am getting this in the console: It looks like the suggestion is to set the |
There was a problem hiding this 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 "}> |
There was a problem hiding this comment.
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
Stand by for second PR with the "change" I propose for the demo