render advertisementFeatures #300
Conversation
| @@ -76,6 +76,11 @@ const commentStyles = css` | |||
| padding-bottom: ${remSpace[1]}; | |||
| `; | |||
|
|
|||
| const advertisementFeatureStyles = css` | |||
| ${styles} | |||
| ${textSans} | |||
There was a problem hiding this comment.
Would it be preferable to use a design system font here?
There was a problem hiding this comment.
I had a problem here as I wanted to keep the headline the same size as other articles but that would require larger textSans sizes in the design system https://theguardian.design/2a1e5182b/p/930d69-typography/b/06df96
There was a problem hiding this comment.
Would be happy to try and add that to the design system if it's useful? I imagine Dotcom could have the same problem when adding glabs
There was a problem hiding this comment.
Is the issue that you need a 34px version of Text Sans, and the largest currently available is 24px?
There was a problem hiding this comment.
yep, I could go into source and find the rem value used for that but the current way would also do that.
There was a problem hiding this comment.
I've opened this on Source.
| `; | ||
| const styles = (design: Design): SerializedStyles => { | ||
| const advertisementFeature = design === Design.AdvertisementFeature | ||
| ? textSans |
There was a problem hiding this comment.
Would it be preferable to use a design system font here? E.g. textSans.medium()?
const font = design === Design.AdvertisementFeature ? textSans.medium() : body.medium();
return css`
${font}
overflow-wrap: break-word;
margin: 0 0 ${remSpace[3]};
`;There was a problem hiding this comment.
textSans.medium() is a lot smaller than headline.medium(). Even the largest textSans is smaller that headline.medium()
| @@ -76,6 +76,11 @@ const media = css` | |||
| } | |||
| `; | |||
|
|
|||
| const advertisementFeature = css` | |||
| ${styles} | |||
| ${textSans} | |||
There was a problem hiding this comment.
Would it be preferable to use a design system font here?
|
@JamieB-gu The larger I've seen us use the older Should this be merged beforehand? |
Render AdvertisementFeatures. Also known as Guardian Labs.
Screenshots