Use CallToActionAtomBlockElement in Hosted Content#15521
Use CallToActionAtomBlockElement in Hosted Content#15521
CallToActionAtomBlockElement in Hosted Content#15521Conversation
deedeeh
changed the title
CallToActionAtomBlockElement in Hosted Content
deedeeh
added
the
maintenance
deedeeh
force-pushed
the
dina/use-call-to-action-block-element
branch
from
617f238 to
b3358f4
Compare
deedeeh
force-pushed
the
dina/use-call-to-action-block-element
branch
from
b3358f4 to
79a77e9
Compare
|
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. |
| url: string; | ||
| image?: string; | ||
| label?: string; | ||
| btnText?: string; |
There was a problem hiding this comment.
I think buttonText is easier to read personally, and in general feel that the previous prop names were more intuitive. We don't need them to match the underlying data
There was a problem hiding this comment.
Ok I did add the previous prop names back. I do agree with you they are clearer. Done 369581a
| const overlayMaskGradientStyles = ( | ||
| angle: string, | ||
| startPosition: number, | ||
| ) => { | ||
| const positions = [0, 8, 16, 24, 32, 40, 48, 56, 64].map( | ||
| (offset) => startPosition + offset, | ||
| ); | ||
| return css` | ||
| mask-image: linear-gradient( | ||
| ${angle}, | ||
| transparent ${positions[0]}px, | ||
| rgba(0, 0, 0, 0.0381) ${positions[1]}px, | ||
| rgba(0, 0, 0, 0.1464) ${positions[2]}px, | ||
| rgba(0, 0, 0, 0.3087) ${positions[3]}px, | ||
| rgba(0, 0, 0, 0.5) ${positions[4]}px, | ||
| rgba(0, 0, 0, 0.6913) ${positions[5]}px, | ||
| rgba(0, 0, 0, 0.8536) ${positions[6]}px, | ||
| rgba(0, 0, 0, 0.9619) ${positions[7]}px, | ||
| rgb(0, 0, 0) ${positions[8]}px | ||
| ); | ||
| `; | ||
| }; | ||
|
|
||
| const blurStyles = css` | ||
| position: absolute; | ||
| inset: 0; | ||
| backdrop-filter: blur(12px) brightness(0.5); | ||
| @supports not (backdrop-filter: blur(12px)) { | ||
| background-color: ${transparentColour( | ||
| sourcePalette.neutral[10], | ||
| 0.7, | ||
| )}; | ||
| } | ||
| ${overlayMaskGradientStyles('180deg', 0)}; | ||
|
|
||
| ${from.mobileLandscape} { | ||
| ${overlayMaskGradientStyles('180deg', 20)}; | ||
| } | ||
|
|
||
| ${from.tablet} { | ||
| ${overlayMaskGradientStyles('180deg', 80)}; | ||
| } | ||
|
|
||
| ${from.desktop} { | ||
| ${overlayMaskGradientStyles('180deg', 100)}; | ||
| } | ||
|
|
||
| ${from.leftCol} { | ||
| ${overlayMaskGradientStyles('180deg', 210)}; | ||
| } | ||
| `; | ||
|
|
||
| const buttonWrapperStyles = css` | ||
| ${blurStyles} | ||
| display: flex; | ||
| position: absolute; | ||
| flex-direction: column; | ||
| justify-content: end; | ||
| align-items: center; | ||
| padding: 0 ${space[2]}px ${space[6]}px; | ||
| bottom: 0; | ||
| left: 0; | ||
| right: 0; | ||
|
|
||
| ${from.tablet} { | ||
| flex-direction: row; | ||
| align-items: flex-end; | ||
| padding: ${space[8]}px ${space[9]}px; | ||
| } | ||
|
|
||
| ${from.desktop} { | ||
| justify-content: start; | ||
| padding: ${space[8]}px ${space[5]}px; | ||
| } | ||
| `; | ||
|
|
||
| const labelStyles = css` | ||
| ${textSansBold28} | ||
| width: 100%; | ||
| margin-bottom: ${space[5]}px; | ||
| color: white; | ||
|
|
||
| ${from.tablet} { | ||
| ${textSansBold34} | ||
| margin: 0; | ||
| padding-right: ${space[8]}px; | ||
| } | ||
|
|
||
| ${from.desktop} { | ||
| width: auto; | ||
| max-width: 621px; | ||
| } | ||
| `; |
There was a problem hiding this comment.
Can we move the styles out of the component itself? It's not the usual pattern in DCR
There was a problem hiding this comment.
That wasn't meant to happen. Done 369581a
| color: white; | ||
|
|
||
| ${from.tablet} { | ||
| ${textSansBold34} |
There was a problem hiding this comment.
As far as I can see, the designs say text sans bold 24 until tablet then text sans bold 28 from there?
There was a problem hiding this comment.
I was looking at a different Figma file and not the latest that was sent to the Hosted Content P&E update chat. Done 369581a
| cssOverrides={css` | ||
| width: 100%; | ||
|
|
||
| ${from.tablet} { | ||
| width: auto; | ||
| } | ||
| `} |
There was a problem hiding this comment.
Ideally we wouldn't use cssOverrides. I know the prop exists but there was some discussion a while ago about removing it in favour of more standard control of button styling in the design system.
Can we apply these using general styles focussed towards this button? Perhaps in the buttonWrapper css?
css`
...
button {
width: 100%;
${from.tablet} {
width: auto;
}
}
`|
|
||
| ${from.desktop} { | ||
| width: auto; | ||
| max-width: 621px; |
There was a problem hiding this comment.
Why 621px? This seems oddly specific!
There was a problem hiding this comment.
I specified that based on the initial Figma file as that's the max-width for the text. When I looked in the latest Figma I can see different values as the following:
I think I need to add a max-width for the text.
| ${from.tablet} { | ||
| ${textSansBold34} | ||
| margin: 0; | ||
| padding-right: ${space[8]}px; |
There was a problem hiding this comment.
That's a large value for padding-right. If this is to restrict the width of the text area, we could use something like a calculation instead to make it more explicit what this is for?
max-width: calc(100% - ${buttonWidth}px - ${buttonPadding}px);Or could make the parent a flex container and specify a flex-basis for both the button and the text
wrapper {
display: flex;
}
label {
flex-basis: 75%;
}
button {
flex-basis: 25%;
}There was a problem hiding this comment.
Why can't we just use a padding-right? The design specifically got 20px between the text and the button for all breakpoints from tablet.
| ${textSansBold28} | ||
| width: 100%; | ||
| margin-bottom: ${space[5]}px; | ||
| color: white; |
There was a problem hiding this comment.
Can we use a palette colour here so we remember to change it later with theming?
| ${from.tablet} { | ||
| flex-direction: row; | ||
| align-items: flex-end; | ||
| padding: ${space[8]}px ${space[9]}px; |
There was a problem hiding this comment.
This is an unusually large amount of padding - is this intended?
There was a problem hiding this comment.
Well that's what I saw in the initial Figma file and also I just checked the latest Figma that you are referring to and there is a 32px top and bottom and 34px right and left but instead I used 36px to be able to use Source as we don't have 34px when we use space.
| 'model.dotcomrendering.pageElements.CallToActionAtomBlockElement', | ||
| ); | ||
|
|
||
| //We need to remove the CTA block element from the blocks that are rendered in the article body, otherwise it will be rendered twice. |
There was a problem hiding this comment.
👍 good explanation of why we're doing something funny here!
2 participants
What does this change?
This PR is to use the
CallToActionAtomBlockElementin Hosted Content and consume the data which should be the last piece of the puzzle to migrate the CTA atom to DCAR which is only used by the Hosted Content.Related PRs guardian/frontend#28653 and #15483
The changes include:
CallToActionAtomBlockElementdata to thehostedArticle.tsandhostedVideo.tsmanual fixtures so it could be rendered in storybook.The next step in the CTA work is to use the
accentColourfor the CTA button in light mode and a neutral colour in dark mode but it's better to do it in a separate PR.Why?
This is part of the Hosted Content migration to DCAR.
Screenshots
I added screenshots with label but we can also have CTA without label, which is the heading.