Add more galleries to gallery layout #14545
Conversation
marjisound
changed the base branch from
main
to
add-more-galleries-component
marjisound
force-pushed
the
add-more-galleries-to-gallery-layout
branch
from
2141dcd to
0b0292b
Compare
marjisound
force-pushed
the
add-more-galleries-to-gallery-layout
branch
4 times, most recently
from
384470c to
0561643
Compare
marjisound
force-pushed
the
add-more-galleries-to-gallery-layout
branch
from
9092681 to
43a5907
Compare
Co-authored-by: Jamie B <53781962+JamieB-gu@users.noreply.github.com>
Co-authored-by: Jamie B <53781962+JamieB-gu@users.noreply.github.com>
Removed and re-organised the styles, and shortened the components. Also hardcoded the heading, data attributes and links because they won't change. Co-authored-by: Marjan Kalanaki <marjan.kalanaki@guardian.co.uk>
marjisound
force-pushed
the
add-more-galleries-to-gallery-layout
branch
from
43a5907 to
5be464d
Compare
| ${grid.column.centre} | ||
| color: ${palette('--caption-text')}; | ||
| text-decoration: none; | ||
| align-self: start; |
There was a problem hiding this comment.
What does this declaration do?
There was a problem hiding this comment.
Without the align-self: start; the <a> tag will take the entire row height which is as tall as the splash card section because by default, grid items have align-self: stretch.
This makes a big area clickable. But I thought it might be more user friendly to only have the text part clickable.
Using align-self: start; stops the anchor tag to stretch.
dotcom-rendering/src/components/FetchMoreGalleriesData.importable.tsx
Outdated
Show resolved
Hide resolved
| ); | ||
|
|
||
| return ( | ||
| <div |
There was a problem hiding this comment.
Is there a reason we need an enclosing div, or can these styles be part of the MoreGalleries component?
There was a problem hiding this comment.
The more galleries is using a padded grid container, so the background colour it has is not covering the whole width. This extra div ensures the background color covers the entire width of the section.
We can move this styling into the MoreGalleries container but I think but we’d likely still need a wrapper div to apply the full-width background colour. What do you think?
| addDiscussionIds( | ||
| data.trails | ||
| .map((trail) => trail.discussion?.discussionId) | ||
| .filter(isNonNullable), | ||
| ); |
There was a problem hiding this comment.
This is a side-effect, so it should be run inside a useEffect1. It can probably be included in the one above that's fetching the data?
Footnotes
| if (error) { | ||
| // Send the error to Sentry and then prevent the element from rendering | ||
| window.guardian.modules.sentry.reportError(error, 'more-galleries'); | ||
| return undefined; |
There was a problem hiding this comment.
Typically null is used when you want to render nothing1:
| return undefined; | |
| return null; |
On a related note, rendering nothing here will collapse the placeholder, causing layout shift. An alternative might be to render an error state that won't cause the UI jump. Perhaps one to discuss?
Footnotes
There was a problem hiding this comment.
Yes lets discuss it tomorrow 👍 I've created this ticket for now #14629
| const image: DCRFrontImage | undefined = trail.masterImage | ||
| ? { | ||
| src: trail.masterImage, | ||
| altText: '', |
There was a problem hiding this comment.
Can we create an issue to look into making alt text available for these images in the data?
There was a problem hiding this comment.
Nice suggestion. Created ticket in #14628
| import { Placeholder } from './Placeholder'; | ||
|
|
||
| type Props = { | ||
| limit: number; // Limit the number of items shown (the api often returns more) |
There was a problem hiding this comment.
Does this need to be a prop? If this is always "5" then we can probably hardcode that in the slice below?
Co-authored-by: Jamie B <53781962+JamieB-gu@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
dotcom-rendering/src/components/FetchMoreGalleriesData.importable.tsx
Dismissed
Show dismissed
Hide dismissed
What does this change?
This PR introduces a new importable component,
FetchMoreGalleriesData, which is responsible for fetching More Galleries data on the client side.Updated the
Placeholdercomponent to accept a Map of heights keyed by breakpoint to account for the varying height of the More Galleries component across different screen sizes. Providing breakpoint-specific heights helps prevent layout shifts while data is loading.The
BaseTrailTypeis also updated with 2 new optional fieldstrailText&galleryCount. The relevant frontend change for this is guardian/frontend#28197Why?
Screenshots
This PR fixes #14311