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
Create new Titlepiece
component 🎡
#11368
base: main
Are you sure you want to change the base?
Conversation
Size Change: +28.8 kB (+3.62%) Total Size: 823 kB
ℹ️ View Unchanged
|
a0dd2f4
to
5fa9f35
Compare
eec5427
to
f6ed50a
Compare
Everythiiing
9fbffbc
to
ab72704
Compare
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
@media (min-width: 375px) and (max-width: 479px) { | ||
height: 34px; | ||
} | ||
@media (min-width: 480px) and (max-width: 739px) { |
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.
I'm interested as to why we're not using Source breakpoints here? Would you care to elaborate? Along the benefits of standardisation, there are also some very narrow edge cases that are covered by the API.
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.
Ah good catch, thank you @mxdvl. Is this the kid of implementation you had in mind: 76b4ce6?
A lot of the design break points are at widths not covered in Source (or are a mix, e.g. a Source-friendly 'from' but a bespoke 'until'). Is there a way to mix the two so we're getting as much standardisation as possible?
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.
Ok, we've clarified that the breakpoints in Figma weren't prescriptive, more 'this is what it would look like at this size'. The design pixel headings have been updated to align with Source and all breakpoints in the PR now use it (7a05dfe)
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.
Might be easier to not include changes to this file in this PR, so that we are purely developing in Storybook until ready to include in the main Masthead
component. This should prevent the linting errors you've got here showing up
* Create a CSS grid container. | ||
* Use jointly with Content, LeftCol & RightCol. | ||
* | ||
* @see https://theguardian.design/2a1e5182b/p/41be19-grids |
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.
👨🍳🤌 ✨
@media (min-width: 480px) and (max-width: 1023px) { | ||
margin-bottom: 10px; | ||
} | ||
@media (min-width: 1024px) and (max-width: 1279px) { | ||
margin-bottom: ${space[3]}; | ||
} | ||
@media (min-width: 1280px) { | ||
margin-bottom: 14px; | ||
} |
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.
As mentioned elsewhere, we should be using source breakpoints for these media queries really.
Another tip is that it is often easier to think of media queries as:
- define default (mobile first is best usually)
- define first breakpoint that changes the default, starting from the lowest viewport size (ie.
from.tablet
). We don't need to saybetween
usually as this is inferred. - define following breakpoints increasing from the previous (i.e.
from.desktop
)...
* | ||
* @see https://theguardian.design/2a1e5182b/p/41be19-grids | ||
*/ | ||
export const TitlepieceGrid = ({ |
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.
This component seems very generic so might be worth considering, possibly in another PR, whether we want to call this "TitlepieceGrid" or some sort or more common "Grid" 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.
Worth noting on this for other reviewers, that this file is largely copied from the HeaderTopBarEditionDropdown
component
url: edition.url, | ||
title: edition.longTitle, | ||
dataLinkName: nestedOphanComponents( | ||
'nav4', |
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.
We're removing "nav4" in favour of "header" which is consistent across web and apps
'nav4', | |
'header', |
title: edition.longTitle, | ||
dataLinkName: nestedOphanComponents( | ||
'nav4', | ||
'topbar', |
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.
'topbar', | |
'titlepiece', |
label={ | ||
isTabletOrSmaller ? activeEdition.id : activeEdition.title | ||
} |
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.
We probably shouldn't have to infer the viewport size via props. I think we could either name the prop something like "hasShortLabel" or we could do something like the following which determines the viewport size within this component rather than passing around a prop with this information.
This is how we might do it using the Hide
component
{/* Version of Dropdown component below tablet size */}
<Hide from="tablet">
<Dropdown ... />
</Hide>
{/* Version of Dropdown component above tablet size */}
<Hide until="tablet">
<Dropdown ... />
</Hide>
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.
That makes sense though just to flag, the viewpoint size is also being used in Titlepiece
to check whether the final pillar link has a line after it or not:
Both use the same windowWidth
value for reference at the moment. Do you reckon it's cleaner to implement them independently using Hide
as above?
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.
Ah yes, let me comment on that scenario separately
const pillarLinkWidth1280 = 125; | ||
const pillarLinkWidth1440 = 136; | ||
|
||
const editionsMenuStyles = css` |
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.
const editionsMenuStyles = css` | |
const editionSwitcherMenuStyles = css` |
this is a really nitpicky terminology thing but "editions" is a term we largely reserve for the "Editions app" to distinguish from any other time we talk about the "edition" of the site or paper
76b4ce6
to
394dfc4
Compare
css={css` | ||
position: relative; | ||
${index > 0 && | ||
`padding-left: ${space[1]}px;`} | ||
`} |
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.
It's much easier to read this css when it's extracted into its own appropriately named const
due to the nesting levels
/> | ||
{(index !== pillars.length - 1 || | ||
(index === pillars.length - 1 && | ||
windowWidth > 1023)) && ( |
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.
I'd advise against accessing the window
object unless you really need to.
In this scenario, we can use CSS media queries directly, instead of using windowWidth
. This is also preferred because it's a no JS solution.
See https://github.com/guardian/dotcom-rendering/blob/main/dotcom-rendering/docs/architecture/021-NoJS-navigation-and-accessibility.md
<div css={guardianLogoStyles}> | ||
<a href="https://theguardian.com"> | ||
<SvgGuardianLogo | ||
textColor={themePalette('--masthead-nav-link-text')} | ||
/> | ||
</a> | ||
</div> |
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.
We should be using Logo
component here since that takes care of the editionalised versions
const [windowWidth, setWindowWidth] = useState(getWindowWidth()); | ||
useEffect(() => { | ||
const handleResize = () => { | ||
setWindowWidth(getWindowWidth()); | ||
}; | ||
window.addEventListener('resize', handleResize); | ||
return () => { | ||
window.removeEventListener('resize', handleResize); | ||
}; | ||
}, []); |
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.
CSS media queries should handle this sufficiently without using React state and effect and resize handlers etc
<div css={editionSwitcherMenuStyles}> | ||
<TitlepieceEditionDropdown | ||
editionId={editionId} | ||
dataLinkName={''} |
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.
/** TODO */
Standardisation, solidarity, liberty
7a05dfe
to
c1b8bf9
Compare
What does this change?
The first step towards adding a fully fledged
Titlepiece
component for the new-look site masthead. This PR creates a new component and focuses on the design, with the appearance adjusting as desired across multiple breakpoints.Implementation build on a CSS Grid approach modelled by @mxdvl during the Basecamp project (for reference), with different parts of the titlepiece positioned on a shifting grid.
Companion to #11190. The contents of this component will be kept out of public view by the same 0% test that the masthead as a whole is sitting behind.
Not covered in this PR but will need to be done in follow-ups:
nav
prop (this should also capture highlighting section colours depending on where the reader is)Plus a bunch of silly mistakes/omissions I've no doubt made.
Why?
As part of the ongoing 'Fairground' project to revamp the homepage, we're taking an incremental approach to the new-look masthead. This PR should lay a base on which we can add further functionality and best practice.
Screenshots