Add header image form to designable banner page #536
Conversation
| @@ -27,6 +27,13 @@ object GuardianRoundelDesign { | |||
| implicit val decoder = deriveEnumerationDecoder[GuardianRoundelDesign] | |||
| } | |||
|
|
|||
| case class HeaderImage( | |||
There was a problem hiding this comment.
did you consider reusing BannerDesignVisual.Image?
There was a problem hiding this comment.
I think I may have dissuaded Rik from that because that type has a kind which isn't used/needed here. What do you think?
| const imageUrlValidation = { | ||
| value: /^https:\/\/i\.guim\.co\.uk\//, | ||
| message: 'Images must be valid URLs hosted on https://i.guim.co.uk/', | ||
| }; |
There was a problem hiding this comment.
Not a big deal, but perhaps this common validation could be shared with the other image fields?
| useEffect(() => { | ||
| const isValid = Object.keys(errors).length === 0; | ||
| onValidationChange('headerImage', isValid); | ||
| console.log(errors); |
There was a problem hiding this comment.
Should this console.log be removed?
public/src/models/bannerDesign.ts
Outdated
| @@ -54,6 +54,7 @@ export type BannerDesignVisual = BannerDesignImage | ChoiceCardsDesign; | |||
|
|
|||
| export type BannerDesignProps = { | |||
| visual?: BannerDesignVisual; | |||
| headerImage?: BannerDesignImage; | |||
There was a problem hiding this comment.
I think this TS type has a kind field, but the Scala case class doesn't. I wonder if that's confusing or going to cause subtle issues anywhere? What do you reckon, should they match?
There was a problem hiding this comment.
Fixed (hopefully)
| @@ -161,7 +170,7 @@ const BannerDesignForm: React.FC<Props> = ({ | |||
|
|
|||
| <Accordion className={classes.accordion}> | |||
| <AccordionSummary className={classes.sectionHeader} expandIcon={<ExpandMoreIcon />}> | |||
| Image or Choice Cards | |||
| Main Image or Choice Cards | |||
| @@ -38,14 +38,17 @@ export interface TickerDesign { | |||
| goalMarker: HexColour; | |||
| } | |||
|
|
|||
| export interface BannerDesignImage { | |||
| kind: 'Image'; | |||
| export interface BannerDesignHeaderImage { | |||
There was a problem hiding this comment.
it's only names and not important, but perhaps it makes more sense to call this just BannerDesignImage and the one that extends it MainBannerDesignImage
What does this change?
Extends the designable banner page to allow users to self-serve a header image
How to test