Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor to make the reminder button configurable #430

Merged
merged 20 commits into from
Nov 13, 2023
Merged

Refactor to make the reminder button configurable #430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
4b78247
wip end-of-day commit
KaliedaRik Oct 27, 2023
31fcac2
merge main into branch
KaliedaRik Nov 1, 2023
d248629
refactor colors
KaliedaRik Nov 1, 2023
23f2423
fix js import errors
KaliedaRik Nov 1, 2023
c521149
Banners and epics will have different style objects
KaliedaRik Nov 1, 2023
cad223e
remove unnecessary code
KaliedaRik Nov 2, 2023
f1563d0
fixing types issues
KaliedaRik Nov 3, 2023
78a2f0f
fix linting
KaliedaRik Nov 3, 2023
e171991
fixes following comments on PR
KaliedaRik Nov 9, 2023
b70f05e
further amendments to types
KaliedaRik Nov 9, 2023
7b23d5b
add inline documentation comment
KaliedaRik Nov 9, 2023
d8323d1
update loading dots animation
KaliedaRik Nov 9, 2023
75e0915
update loading dots animation
KaliedaRik Nov 9, 2023
c7a1cbb
fix linting
KaliedaRik Nov 9, 2023
6d5aa3c
fix linting
KaliedaRik Nov 13, 2023
8a09e5c
Refactor getColors to be generic
tjmw Nov 13, 2023
45e1323
Merge pull request #431 from guardian/tw/get-colors-generics
KaliedaRik Nov 13, 2023
345d7d4
Add some tests for getColors
tjmw Nov 13, 2023
0406c5f
Merge pull request #432 from guardian/tw/get-colors-generics
KaliedaRik Nov 13, 2023
7b52fad
update stories
KaliedaRik Nov 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/AppBanner/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,15 @@ import type { TrackClick } from '../utils/tracking';

import { AppStore } from '../assets/app-store';
import { PlayStore } from '../assets/play-store';
import { StyleData, selfServeStyles } from '../styles/bannerCommon';
import { BannerColorStyles } from '../styles/colorData';
import { selfServeStyles } from '../styles/bannerCommon';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a thing that's been confusing me about the configurable colours is that AppBanner, BannerNewsletter & friends, which don't support configuring the colours, still call selfServeStyles. I think that's because selfServeStyles serves two purposes:

  1. Takes user provided styles, and integrates these with defaults (if, for example, the user provides invalid values, or if certain colours are missing)
  2. Generates CSS from a the colour styles object.

In the case of the banners which don't have configurable colours, we only need the second of those two purposes. This could be done in a follow up PR, but I think it would improve the understandability of the code to de-couple those two things. So instead of selfServeStyles taking user provided colours and defaults it just takes colours which are guaranteed to be present and returns the CSS. It wouldn't have to care about where the colours came from (for AppBanner etc they'd be hardcoded, for StyleableBannnerWithLink they come from combining user provided colours with defaults). At the top level of StyleableBannerWithLink it'd have to do the combining of user provided colours and defaults, using getColors, and selfServeStyles wouldn't call getColors.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My long-term utopia is to have a single epic component, and a single banner component (both styleable, though in the case of the epic limited to eg CTA button colours) which Retention can setup/preview in Storybook. Though the competing vision of being able to use RRCP banner/epic designs from Braze is an even better utopia destination.

import { useEscapeShortcut, OnCloseClick, CLOSE_BUTTON_ID } from '../bannerCommon/bannerActions';
import { styles } from './styles';

import { canRender, COMPONENT_NAME } from './canRender';
export { COMPONENT_NAME };

const defaultColors: StyleData = {
const defaultColors: BannerColorStyles = {
styleBackground: '#ebe8e8',
styleHeader: `#333333`,
styleBody: '#666',
Expand Down
5 changes: 3 additions & 2 deletions src/BannerNewsletter/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import { useEscapeShortcut, OnCloseClick, CLOSE_BUTTON_ID } from '../bannerCommo
import { CTA, NewsletterFrequency } from '../newsletterCommon';
import { NewsletterSubscribeCallback } from '../types/dcrTypes';
import type { TrackClick } from '../utils/tracking';
import { StyleData, selfServeStyles } from '../styles/bannerCommon';
import { BannerColorStyles } from '../styles/colorData';
import { selfServeStyles } from '../styles/bannerCommon';
import { canRender, COMPONENT_NAME } from './canRender';
export { COMPONENT_NAME };

Expand All @@ -30,7 +31,7 @@ const localStyles = {
`,
};

const defaultColors: StyleData = {
const defaultColors: BannerColorStyles = {
styleBackground: '#ebe8e8',
styleHeader: `#333333`,
styleBody: '#666',
Expand Down
5 changes: 3 additions & 2 deletions src/BannerWithLink/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@ import React, { useState } from 'react';
import { Button, LinkButton, SvgCross } from '@guardian/source-react-components';
import { useEscapeShortcut, OnCloseClick, CLOSE_BUTTON_ID } from '../bannerCommon/bannerActions';
import type { TrackClick } from '../utils/tracking';
import { StyleData, selfServeStyles } from '../styles/bannerCommon';
import { BannerColorStyles } from '../styles/colorData';
import { selfServeStyles } from '../styles/bannerCommon';
import { canRender, COMPONENT_NAME } from './canRender';
export { COMPONENT_NAME };

const defaultColors: StyleData = {
const defaultColors: BannerColorStyles = {
styleBackground: '#ebe8e8',
styleHeader: `#333333`,
styleBody: '#666666',
Expand Down
2 changes: 1 addition & 1 deletion src/Epic/failure.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export default {
...ophanComponentIdArgType,
heading: {
name: 'heading',
type: { name: 'string', required: true },
type: { name: 'string', required: false },
description: 'Header text',
},
...Object.fromEntries(paragraphDocs),
Expand Down
2 changes: 1 addition & 1 deletion src/Epic/index.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export default {
...ophanComponentIdArgType,
heading: {
name: 'heading',
type: { name: 'string', required: true },
type: { name: 'string', required: false },
description: 'Header text',
},
...Object.fromEntries(paragraphDocs),
Expand Down
10 changes: 10 additions & 0 deletions src/Epic/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { replaceNonArticleCountPlaceholders } from './placeholders';
import { TrackClick } from '../utils/tracking';
import { FetchEmail } from '../types/dcrTypes';
import { ReminderStage } from '../logic/reminders';
import { ReminderButtonColorStyles } from '../styles/colorData';
import { HeaderSection } from './HeaderSection';

// Custom styles for <a> tags in the Epic content
Expand Down Expand Up @@ -64,6 +65,13 @@ const styles = {
`,
};

const defaultColors: ReminderButtonColorStyles = {
styleReminderButton: '#121212',
styleReminderButtonBackground: '#ededed',
styleReminderButtonHover: '#dcdcdc',
styleReminderAnimation: '#707070',
};

export type BrazeMessageProps = {
buttonText?: string;
buttonUrl?: string;
Expand Down Expand Up @@ -171,11 +179,13 @@ export const Epic: React.FC<EpicProps> = (props: EpicProps) => {
/>
{reminderStage && (
<ReminderCtaButton
reminderComponent="EPIC"
reminderStage={reminderStage}
reminderOption={reminderOption}
ophanComponentId={ophanComponentId as string}
trackClick={trackClick}
fetchEmail={fetchEmail}
userStyles={defaultColors}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about calling this prop colors or similar? I think userStyles isn't always accurate (in this case they're hardcoded for example).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take this forward in the next PR

/>
)}
</div>
Expand Down
16 changes: 15 additions & 1 deletion src/EpicWithSpecialHeader/index.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export default {
},
heading: {
name: 'heading',
type: { name: 'string', required: true },
type: { name: 'string', required: false },
description: 'Copy header text',
},
...Object.fromEntries(paragraphDocs),
Expand All @@ -83,6 +83,16 @@ export default {
type: { name: 'string', required: false },
description: 'Set value to "true" to suppress the payment icons beneath the CTA',
},
reminderStage: {
name: 'reminderStage',
type: { name: 'string', required: false },
description: 'The type of stage (PRE, POST, WINBACK)',
},
reminderOption: {
name: 'reminderOption',
type: { name: 'string', required: false },
description: 'Extra data to be associated with a reminder sign-up',
},
},
};

Expand All @@ -100,6 +110,8 @@ const StoryTemplate = (
buttonText: args.buttonText,
buttonUrl: args.buttonUrl,
hidePaymentIcons: args.hidePaymentIcons,
reminderStage: args.reminderStage,
reminderOption: args.reminderOption,
paragraph1: args.paragraph1,
paragraph2: args.paragraph2,
paragraph3: args.paragraph3,
Expand Down Expand Up @@ -161,6 +173,8 @@ DefaultStory.args = {
ophanComponentId: 'example_ophan_component_id',
slotName: 'EndOfArticle',
componentName: 'EpicWithSpecialHeader',
reminderStage: 'PRE',
reminderOption: 'recurring-contribution-upsell',
};

DefaultStory.story = { name: 'EpicWithSpecialHeader' };
30 changes: 20 additions & 10 deletions src/StyleableBannerWithLink/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,36 +2,42 @@ import React, { useState } from 'react';
import { Button, LinkButton, SvgCross } from '@guardian/source-react-components';
import { useEscapeShortcut, OnCloseClick, CLOSE_BUTTON_ID } from '../bannerCommon/bannerActions';
import type { TrackClick } from '../utils/tracking';
import { StyleData, selfServeStyles } from '../styles/bannerCommon';
import { StyleableBannerColorStyles } from '../styles/colorData';
import { selfServeStyles } from '../styles/bannerCommon';
import { canRender, COMPONENT_NAME } from './canRender';
import { PaymentIcons } from '../components/PaymentIcons';
export { COMPONENT_NAME };

export type BrazeMessageProps = {
ophanComponentId?: string;
styleBackground?: string;
header?: string;
styleHeader?: string;
body?: string;
styleBody?: string;
highlight?: string;
styleHighlight?: string;
styleHighlightBackground?: string;
buttonText?: string;
buttonUrl?: string;
showPaymentIcons?: string;
styleButton?: string;
styleButtonBackground?: string;
styleButtonHover?: string;
includeReminderCta?: string;
imageUrl?: string;
imageAltText?: string;
imagePosition?: string;
styleBackground?: string;
styleHeader?: string;
styleBody?: string;
styleHighlight?: string;
styleHighlightBackground?: string;
styleButton?: string;
styleButtonBackground?: string;
styleButtonHover?: string;
styleReminderButton?: string;
styleReminderButtonBackground?: string;
styleReminderButtonHover?: string;
styleReminderAnimation?: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea to group these together, I think it makes it easier to understand what's configurable colour-wise here 👍

styleClose?: string;
styleCloseBackground?: string;
styleCloseHover?: string;
};

const defaultColors: StyleData = {
const defaultColors: StyleableBannerColorStyles = {
styleBackground: '#ededed',
styleHeader: '#333333',
styleBody: '#333333',
Expand All @@ -40,6 +46,10 @@ const defaultColors: StyleData = {
styleButton: '#ffffff',
styleButtonBackground: '#052962',
styleButtonHover: '#234b8a',
styleReminderButton: '#121212',
styleReminderButtonBackground: '#ededed',
styleReminderButtonHover: '#dcdcdc',
styleReminderAnimation: '#707070',
styleClose: '#052962',
styleCloseBackground: '#ededed',
styleCloseHover: '#e5e5e5',
Expand Down
4 changes: 2 additions & 2 deletions src/canRender.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import type { Extras } from './logic/types';

// Banners
// --------------------------------------------
import {
Expand Down Expand Up @@ -73,8 +75,6 @@ import {
// These are in a seperate file to enable tree shaking of the logic deciding if a Braze message can be rendered
// + This means the user won't download the Braze components bundle when the component can't be shown.

export type Extras = Record<string, string>;

const COMPONENT_CAN_RENDER_MAPPINGS: Record<
string,
(brazeMessageProps: Record<string, unknown>) => boolean
Expand Down
20 changes: 2 additions & 18 deletions src/components/ContributionCtaButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import React from 'react';
import { css, ThemeProvider } from '@emotion/react';
import { neutral, brandAlt, space } from '@guardian/source-foundations';
import { space } from '@guardian/source-foundations';
import { LinkButton } from '@guardian/source-react-components';

import { PaymentIcons } from './PaymentIcons';
import { TrackClick } from '../utils/tracking';
import { contributionsTheme } from '../styles/colorData';

const buttonWrapperStyles = css`
margin: ${space[4]}px ${space[2]}px ${space[1]}px 0;
Expand All @@ -19,23 +20,6 @@ const buttonMargins = css`
margin: ${space[1]}px ${space[2]}px ${space[1]}px 0;
`;

// Custom theme for Button/LinkButton
// See also `tertiaryButtonOverrides` below.
const buttonStyles = {
textPrimary: neutral[7],
backgroundPrimary: brandAlt[400],
backgroundPrimaryHover: brandAlt[300],
textSecondary: neutral[7],
backgroundSecondary: neutral[93],
backgroundSecondaryHover: neutral[86],
borderSecondary: neutral[86],
};

const contributionsTheme = {
button: buttonStyles,
link: buttonStyles,
};

interface ContributionCtaButtonProps {
buttonText: string;
buttonUrl: string;
Expand Down
79 changes: 39 additions & 40 deletions src/components/CtaLoadingDotsAnimation.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React from 'react';
import { css, keyframes } from '@emotion/react';
import type { LoadingDotsColorStyles } from '../styles/colorData';

// CTA dots animation
// -------------------------------------------------------
Expand Down Expand Up @@ -36,44 +37,42 @@ const loadingAnimCss = css`
}
`;

export const LoadingDots = (): React.ReactElement => {
return (
<svg
width="50"
height="17"
viewBox="-5 -5 45 12"
fill="none"
xmlns="http://www.w3.org/2000/svg"
css={loadingAnimCss}
>
<g id="Dots step 1">
<g id="Group 660">
<circle
id="dot_1"
opacity="0.5"
cx="3.0152"
cy="3.56641"
r="3"
fill="#707070"
/>
<circle
id="dot_2"
opacity="0.5"
cx="17.3748"
cy="3.56641"
r="3"
fill="#707070"
/>
<circle
id="dot_3"
opacity="0.5"
cx="31.7348"
cy="3.56641"
r="3"
fill="#707070"
/>
</g>
export const LoadingDots = ({ styleReminderAnimation }: LoadingDotsColorStyles) => (
<svg
width="50"
height="17"
viewBox="-5 -5 45 12"
fill="none"
xmlns="http://www.w3.org/2000/svg"
css={loadingAnimCss}
>
<g id="Dots step 1">
<g id="Group 660">
<circle
id="dot_1"
opacity="0.5"
cx="3.0152"
cy="3.56641"
r="3"
fill={styleReminderAnimation}
/>
<circle
id="dot_2"
opacity="0.5"
cx="17.3748"
cy="3.56641"
r="3"
fill={styleReminderAnimation}
/>
<circle
id="dot_3"
opacity="0.5"
cx="31.7348"
cy="3.56641"
r="3"
fill={styleReminderAnimation}
/>
</g>
</svg>
);
};
</g>
</svg>
);
Loading
Loading