Create new Titlepiece component 🎡
#11368
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +28.8 kB (+3.62%) Total Size: 823 kB
ℹ️ View Unchanged
|
frederickobrien
force-pushed
the
fairground-titlepiece
branch
from
a0dd2f4
to
5fa9f35
Compare
frederickobrien
force-pushed
the
fairground-titlepiece
branch
2 times, most recently
from
eec5427
to
f6ed50a
Compare
Everythiiing
frederickobrien
force-pushed
the
fairground-titlepiece
branch
from
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. |
mxdvl
reviewed
May 21, 2024
| @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.
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.
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.
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)
cemms1
reviewed
May 22, 2024
There was a problem hiding this comment.
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 |
Comment on lines
104
to
112
| @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.
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 saybetweenusually 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.
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.
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.
We're removing "nav4" in favour of "header" which is consistent across web and apps
Suggested change
| 'nav4', | |
| 'header', |
| title: edition.longTitle, | ||
| dataLinkName: nestedOphanComponents( | ||
| 'nav4', | ||
| 'topbar', |
There was a problem hiding this comment.
Suggested change
| 'topbar', | |
| 'titlepiece', |
Comment on lines
58
to
60
| label={ | ||
| isTabletOrSmaller ? activeEdition.id : activeEdition.title | ||
| } |
There was a problem hiding this comment.
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.
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.
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.
Suggested change
| 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
frederickobrien
force-pushed
the
fairground-titlepiece
branch
from
76b4ce6
to
394dfc4
Compare
cemms1
reviewed
May 22, 2024
Comment on lines
+297
to
+301
| css={css` | ||
| position: relative; | ||
| ${index > 0 && | ||
| `padding-left: ${space[1]}px;`} | ||
| `} |
There was a problem hiding this comment.
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.
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
Comment on lines
+269
to
+275
| <div css={guardianLogoStyles}> | ||
| <a href="https://theguardian.com"> | ||
| <SvgGuardianLogo | ||
| textColor={themePalette('--masthead-nav-link-text')} | ||
| /> | ||
| </a> | ||
| </div> |
There was a problem hiding this comment.
We should be using Logo component here since that takes care of the editionalised versions
Comment on lines
+247
to
+256
| 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.
CSS media queries should handle this sufficiently without using React state and effect and resize handlers etc
| <div css={editionSwitcherMenuStyles}> | ||
| <TitlepieceEditionDropdown | ||
| editionId={editionId} | ||
| dataLinkName={''} |
Standardisation, solidarity, liberty
frederickobrien
force-pushed
the
fairground-titlepiece
branch
from
7a05dfe
to
c1b8bf9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this change?
The first step towards adding a fully fledged
Titlepiececomponent 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:
navprop (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