Conversation
| const HOSTED_CONTENT_HEIGHT_MOBILE = 48; | ||
| const HOSTED_CONTENT_HEIGHT_DESKTOP = 58; |
There was a problem hiding this comment.
These values are only used in one place: the headerWrapperStyles below so don't need to be defined as constants at the top of the file
| width: ${breakpoints.desktop}px; | ||
| } | ||
|
|
||
| ${from.leftCol} { | ||
| max-width: 71.25rem; | ||
| width: ${breakpoints.leftCol}px; | ||
| } | ||
|
|
||
| ${from.wide} { | ||
| max-width: 81.25rem; | ||
| width: ${breakpoints.wide}px; |
There was a problem hiding this comment.
Prefer to use the breakpoint definitions directly rather than to hardcode rem values
| /** Hard-coded to fit. TODO - address this */ | ||
| font-size: 13px; |
There was a problem hiding this comment.
Need to talk to design about this. The designs don't work in practice
| height: auto; | ||
| top: 100%; | ||
| text-align: center; | ||
| z-index: 10; |
There was a problem hiding this comment.
10 seems a bit arbitrary. We should only need 1 to ensure it falls on top of the main content. If using a value other than 1, better to use the function getZIndex since this is more predictable across pages compared to manually specifying z-index values
| > | ||
| <HostedContentHeader | ||
| branding={branding} | ||
| accentColor={branding.hostedCampaignColour} |
There was a problem hiding this comment.
Since we already have access to hostedCampaignColour in the branding object, there's no need to pass it through as a separate prop
cemms1
force-pushed
the
cemms1/simplify-hosted-header
branch
from
a4e7d71 to
95ab406
Compare
github-actions
bot
removed
the
run_chromatic
…ts in the branding object
…ponent that wraps the header
… header component itself
cemms1
force-pushed
the
cemms1/simplify-hosted-header
branch
from
95ab406 to
80b5b54
Compare
github-actions
bot
removed
the
run_chromatic
1 participant
What does this change?
headerelement as the semantic HTML element for the header in theSectionparent component for the Hosted content page layoutsHostedContentHeaderupdates:iicon button to the advertisement content labeldivelements from the DOM tree forHostedContentHeaderWhy?
Tweaking the page layouts for the Hosted Content pages to bring them closer to our new, reworked designs for these pages for the migration
Screenshots