Lift ImageDetails To common-rendering
#3520
Conversation
- Renamed `HeaderImageCaption` to `ImageDetails` - Simplified component API - Broke up component CSS - Added type definitions to common - Lifted `pipe` and `maybeRender` to common
JamieB-gu
requested review from
OllysCoding,
marjisound,
lucymonie and
nitro-marky
|
Size Change: +438 B (0%) Total Size: 959 kB
ℹ️ View Unchanged
|
| import { brandAlt, neutral } from '@guardian/src-foundations/palette'; | ||
| import { textSans } from '@guardian/src-foundations/typography'; | ||
| import { SvgCamera } from '@guardian/src-icons'; | ||
| import type { Option } from '@guardian/types'; |
There was a problem hiding this comment.
this package has been deprecated in favour of libs and now only contains Option and Result.
they're left installable (but deprecated) for existing codebases that still need use them, but they shouldn't really be used in new ones.
this is especially true of things that intended to be shared from the start (cf guardian/types#12 (review))
There was a problem hiding this comment.
I strongly agree with @sndrs. We shouldn't be propagating a deprecated library. It is no longer possible to make changes to @guardian/types which puts this repo at further risk of exposure to unfixable bugs and security vulnerabilities. It also frustrates our team's efforts to help you migrate away from this package.
There was a problem hiding this comment.
just to reiterate the auto-dismissed comments, @guardian/types is deprecated in favour of @guardian/libs, and use of Option/Result is strongly discouraged for all the reasons they weren't ported over to libs.
we might not be looking to remove them from existing code in situ, but this is altering the intention and location of that existing code into sharable behaviours intended to be used outside the current AR codebase.
JamieB-gu
dismissed
sndrs’s stale review
As discussed offline, I'd like to get this merged because it fixes an issue in production (see final commit). We're going to discuss Alex's concerns separately.
Why?
AR uses a combination of a camera icon and
summary/details(see screenshots) to handle image captions. Design would like to start using this on dotcom too, starting with the liveblogs. This PR lifts up and refactors the component that manages this.FYI @HarryFischer
Changes
HeaderImageCaptionto common and renamed toImageDetailspipeandmaybeRenderto common (will need a follow-up at some point to update all the imports, I've just re-exported for now to keep this PR small)Screenshots