-
Notifications
You must be signed in to change notification settings - Fork 30
Add MoreGalleries component with a story #14538
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
base: main
Are you sure you want to change the base?
Conversation
85206ec
to
3755b1f
Compare
trailTextColour?: string; | ||
/** Controls visibility of trail text on various breakpoints */ | ||
hideUntil?: 'tablet' | 'desktop'; | ||
hideUntil?: 'tablet' | 'desktop' | 'never'; |
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.
Why did I need a never
value?
The hideUntil
can be undefined. When it's undefined, the component will always add the trail text, but in css display
is set to none
for viewport below tablet
.
When hideUntil
is not undefined, the trail text will be wrapped within a Hide
element and become hidden until the viewport that's given by the hideUntil. However, the css display: none;
is also set for until.tablet
in this case.
So no matter what, we always have display: none;
on trail text for viewport below tablet
. But in our case for more-galleries onward container, we want to always display trail text within the flexSplash card for all viewports.
That's why I added the never
value in order to always display the trail for all viewports.
@arelra and @abeddow91 what do you think about this change?
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 wonder if it might be simpler to introduce an optional prop such as:
hide: boolean
which defaults to true
This then controls whether the trail text applies the <Hide>
component or not.
In existing usages of TrailText
this will default to true and apply <Hide>
but for the galleries onwards container we could pass hide=false
which would mean the hiding behaviour would not apply ?
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.
Great idea, I instead called the new param as hideOnSmallBreakpoints
to make it clearer what it's doing, is that OK?
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.
Having thought about this a little more I'm not sure I understand why we don't use the absence or presence of hideUntil
to control whether hiding is activated?
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.
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. |
Co-authored-by: Jamie B <53781962+JamieB-gu@users.noreply.github.com>
3755b1f
to
367a603
Compare
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 looks good. Just one suggestion.
Instead always pass hideUntil when hiding is required and pass undefined when hiding is not required (trail text needs to be visible for all breakpoints) Co-authored-by: Ravi <7014230+arelra@users.noreply.github.com>
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.
Great work! ✨
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.
Looks good! Some questions and suggestions.
}, | ||
'--onward-background': { | ||
light: () => sourcePalette.neutral[100], | ||
dark: () => sourcePalette.neutral[0], |
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 definitely neutral.0
? The rest of the article, including the other onward content, uses neutral.10
in dark mode.
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.
The background colour for dark mode seems to be background: var(--Neutral-neutral-neutral-0, #000);
in figma.
but the article itself has neutral.10 in the middle part and as you said the other onward content also use neutral.10.
I'll change it to neutral.10 for now but will raise this with the designer to confirm.
discussionApiUrl: | ||
'https://discussion.code.dev-theguardian.com/discussion-api', | ||
heading: 'More galleries', | ||
url: 'http://localhost:9000/more-galleries', |
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.
Should this be pointing to localhost
? It looks like this is just used for the heading link, in which case it's probably fine to use the PROD URL, which on frontend is https://www.theguardian.com/inpictures/all I think?
onwardsSource: 'more-galleries', | ||
trails: [ | ||
{ | ||
url: 'http://localhost:9000/environment/gallery/2025/aug/22/week-in-wildlife-a-clumsy-fox-swinging-orangutang-and-rescued-jaguarundi-cub', |
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.
Similar point on localhost
for each of these trail URLs. If these are just used for links it's probably fine to use the PROD URL. That way people viewing this don't need to be running frontend locally to follow them.
kickerText: 'Politics', // Get data for this | ||
mainMedia: { type: 'Gallery', count: '6' }, // TODO: get data for this |
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.
Are the TODOs in this file still to do?
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.
Good spot, the todos have already been dealt with 👍
heading: string; | ||
onwardsSource: OnwardsSource; |
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 it possible for this to have a heading
that isn't "More galleries" or an onwardsSource
that isn't "more-galleries"? If not we can probably hardcode these and remove the props?
discussionApiUrl: string; | ||
heading: string; | ||
onwardsSource: OnwardsSource; | ||
url?: string; |
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.
Small thing, but perhaps this could be called something like headingLink
? At first glance it may not be obvious what a url
would be for in this component?
&::first-letter { | ||
text-transform: capitalize; | ||
} |
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.
If we hardcode to "More galleries", as mentioned above, we can probably remove this?
{Card({ | ||
...getDefaultCardProps( | ||
trail, | ||
props.absoluteServerTimes, | ||
props.discussionApiUrl, | ||
), | ||
mediaSize: 'medium', | ||
})} |
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.
Although React components are defined as functions, it's best to use createElement
(i.e. JSX) rather than call them directly1.
{Card({ | |
...getDefaultCardProps( | |
trail, | |
props.absoluteServerTimes, | |
props.discussionApiUrl, | |
), | |
mediaSize: 'medium', | |
})} | |
<Card | |
{...getDefaultCardProps( | |
trail, | |
props.absoluteServerTimes, | |
props.discussionApiUrl, | |
)} | |
mediaSize="medium" | |
/> |
Footnotes
padding: ${space[2]}px; | ||
`} | ||
> | ||
{Card({ ...defaultProps, ...cardProps })} |
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 above, it's best to use createElement
(JSX) to render React components:
{Card({ ...defaultProps, ...cardProps })} | |
<Card {...defaultProps} {...cardProps} /> |
uniqueId?: string; | ||
/** The Splash card in a flexible container gets a different visual treatment to other cards */ | ||
/** The Splash card in a flexible or onward container gets a different visual treatment to other cards */ | ||
isFlexSplash?: boolean; |
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 wonder if we should rename this field or use a seperate one here. I'm not sure we should be coupling a flexible splash card with an onwards container splash, unless they are truely the same card. If they are then perhaps renaming to something more generic (ie removing the flexible part would be preferable).
Does the onwards "splash" card share the same characteristics as a flexible splash card?
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.
Thanks for pointing this out @abeddow91
You are right, it seems the characteristics of a flexible splash card is different from an onward splash card.
The isFlexSplash
is primarily used to deal with positioning the sub links. But what's important in an onward splash card is that it needs to have the trail text while the non-splash cards do not have trail text.
I created a new prop isOnwardSplash
for this in here 73cd910
Paired with @JamieB-gu
What does this change?
This PR creates a new MoreGalleries component which will be used for the more galleries onward content section on gallery articles. For this component we were given new design that is different from how this section looks currently in production.
This new component is not currently used. The usage will be implemented in an upcoming PR.
Screenshots
This PR fixes #14310