Refactor to make the reminder button configurable #430

KaliedaRik · 2023-10-27T16:09:00Z

What does this change?

This PR refactors some code which will make the work of adding the reminders button to the styleable banner component easier.

The changes are:

A new styles/colorData.ts file to colocate colour styling related types and functions in one place.
The colour-related types and objects, and functionality to validate user-supplied colour strings, has been moved out of styles/bannerCommon.ts into the new file. The AppsBanner, BannerNewsletter, BannerWithLink, Epic and StyleableBannerWithLink components have been updated to reflect these changes, as has the ContributionCtaButton component.
The components/CtaLoadingDotsAnimation component has been updated to return a function to output the SVG, which can accept a colour string argument. The newsletterCommon/index.tsx and components/ReminderCtaButton.tsx components have been updated to use the function
Most of the refactor in this PR has been to the components/ReminderCtaButton.tsx component. These changes mean that the component can now be styled by user-defined colour strings, with default values for those strings based on the existing colours of the reminder button. The colour types and objects now in the styles/colorData.ts file have been updated to include these new optional attributes.

How to test

All chromatic tests should pass straight away as this PR is an internal refactor which shouldn't affect graphical output.

tjmw

Nice refactoring! Good shout to make this a separate PR. I've left some comments/suggestions on the types, see what you think.

tjmw · 2023-11-07T16:14:26Z

src/components/CtaLoadingDotsAnimation.tsx

@@ -36,7 +36,7 @@ const loadingAnimCss = css`
    }
 `;

-export const LoadingDots = (): React.ReactElement => {
+export const buildLoadingDots = (fillStyle = '#707070'): React.ReactElement => {


Would it be more conventional maybe to have this continue to be named LoadingDots (it is a component after all, not a function which returns a component)? The fillStyle could be passed as an (optional) prop, which would allow it to be called in the normal way: <LoadingDots fillStyle="#121212" />. What do you think?

Current code matches the factory approach we have to generating CSS, but I'm happy to turn this into a component. Will action.

tjmw · 2023-11-07T16:29:19Z

src/components/ReminderCtaButton.tsx

-        margin-top: ${space[2]}px;
-        ${body.small()};
-    `,
+const getButtonStyles = (userVals: Extras, defaults: ColorStylesData) => {


I wonder if we could be more descriptive/specific with the types of these args. Extras is very generic and doesn't provide much information about what might be available. My understanding is getColors takes some colours which might be provided and some defaults which will be provided and comes up with a new instance of ColorStylesData which is guaranteed to have specific fields set (if we can't get the colour from the user provided values then fall back on the defaults).

What if the type of the user provided styles was called something like UserDefinedColorStylesData and it looked something like this (clipped for brevity):

export interface UserDefinedColorStylesData { styleBackground?: string; styleHeader?: string; styleBody?: string; }

So any of those fields may be there, but could also be missing. I think this tells us more about the possible user provided styles than Extras does. Then we could define ColorStylesData as the type which is guaranteed to have all these fields:

type ColorStylesData = Required<UserDefinedColourStylesData>;

Then the type of getColors becomes: (userVals: UserDefinedColorStylesData, defaults: ColorStylesData): ColorStylesData.

Will take this suggestion forward. We can pair if I run into TS issues?

If we can get rid of Extras ... I wouldn't cry. I think there's another type with the same definition somewhere.

We can pair if I run into TS issues?

Definitely! 👍

I've given it a go. Discuss tomorrow?

tjmw · 2023-11-07T16:38:17Z

src/components/ReminderCtaButton.tsx

-        ${body.small()};
-    `,
+const getButtonStyles = (userVals: Extras, defaults: ColorStylesData) => {
+    const styles = getColors(userVals as Extras, defaults);


Currently the return value of getColours is ColorStylesData and all of the fields in that type are optional - but we interpolate them into the CSS styles below as though they are guaranteed to be present. I think returning a type where the fields are guaranteed to be there would be a win in terms of type safety (as in the comment above). What do you reckon? Happy to chat this over/pair if it's useful!

As mentioned above, will give it a go

tjmw · 2023-11-07T16:40:01Z

src/components/ReminderCtaButton.tsx

 interface ReminderCtaButtonProps {
+    reminderComponent: ReminderComponent;


tjmw

This is looking good. I've left a bunch of feedback, see what you think. If we wanted to action the points it could be done on this PR or in a follow up. Happy to chat if anything doesn't make sense (or if I've missed the point with any of it).

tjmw · 2023-11-10T15:37:50Z

src/StyleableBannerWithLink/index.tsx

+    styleReminderButton?: string;
+    styleReminderButtonBackground?: string;
+    styleReminderButtonHover?: string;
+    styleReminderAnimation?: string;


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

tjmw · 2023-11-10T15:44:20Z

src/styles/colorData.ts

+    styleCloseHover: ColorValueHex;
+}
+
+export interface StyleableBannerColorStyles extends ReminderButtonColorStyles, BannerColorStyles {}


Is this interface implemented like this because only the styleable banner supports the reminder buttons? And other banners don't?

tjmw · 2023-11-10T16:04:08Z

src/AppBanner/index.tsx

@@ -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';


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:

Takes user provided styles, and integrates these with defaults (if, for example, the user provides invalid values, or if certain colours are missing)

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?

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.

tjmw · 2023-11-10T16:16:29Z

src/styles/colorData.ts

+
+type ColorStylesType = keyof AllAvailableColorStyles;
+
+export function getColors(userVals: Extras, defaults: Partial<AllAvailableColorStyles>) {


I wonder if using generics here would be a better way to describe the interface of getColours? I don't think getColors would ever be called with an object which had properties of both BannerColorStyles and EpicColorStyles, hence the need for Partial. If it was generic it called be called concretely with the specific relevant type acknowledging the fact that it would be called with a specific concrete type:

function getColors(userVals: Extras, defaults: T): T { // ... } // then called like: getColors<BannerColorStyles>(userVals, defaultBannerColours) getColors<EpicColorStyles>(userVals, defaultEpicColors) /// etc

We've agreed to look at this in a future PR

tjmw · 2023-11-10T16:18:17Z

src/Epic/index.tsx

                                reminderStage={reminderStage}
                                reminderOption={reminderOption}
                                ophanComponentId={ophanComponentId as string}
                                trackClick={trackClick}
                                fetchEmail={fetchEmail}
+                                userStyles={defaultColors}


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

I'll take this forward in the next PR

tjmw · 2023-11-10T16:25:56Z

src/components/ReminderCtaButton.tsx

+    userVals: Partial<ReminderButtonColorStyles>,
+    defaults: ReminderButtonColorStyles,
+) => {
+    const styles = getColors(userVals as Partial<ReminderButtonColorStyles>, defaults);


As in the comment above on selfServeStyles I wonder if it'd be better to decouple the two concerns here (merging user provided vals and defaults) and returning CSS. In some cases there won't be user provided styles as they're hardcoded/not configurable. So perhaps the places that need to can do the combining and then call this function with a complete set of colours.

Refactor getColors to be generic

Add some tests for getColors

wip end-of-day commit

4b78247

KaliedaRik marked this pull request as draft October 27, 2023 16:10

KaliedaRik added 7 commits November 1, 2023 12:30

merge main into branch

31fcac2

refactor colors

d248629

fix js import errors

23f2423

Banners and epics will have different style objects

c521149

remove unnecessary code

cad223e

fixing types issues

f1563d0

fix linting

78a2f0f

KaliedaRik marked this pull request as ready for review November 6, 2023 12:34

tjmw reviewed Nov 7, 2023

View reviewed changes

KaliedaRik added 6 commits November 9, 2023 14:31

fixes following comments on PR

e171991

further amendments to types

b70f05e

add inline documentation comment

7b23d5b

update loading dots animation

d8323d1

update loading dots animation

75e0915

fix linting

c7a1cbb

tjmw approved these changes Nov 10, 2023

View reviewed changes

KaliedaRik and others added 6 commits November 13, 2023 12:52

fix linting

6d5aa3c

Refactor getColors to be generic

8a09e5c

Merge pull request #431 from guardian/tw/get-colors-generics

45e1323

Refactor getColors to be generic

Add some tests for getColors

345d7d4

Merge pull request #432 from guardian/tw/get-colors-generics

0406c5f

Add some tests for getColors

update stories

7b52fad

KaliedaRik merged commit ba0b1bb into main Nov 13, 2023
5 checks passed

KaliedaRik deleted the rjr-braze-banner-reminder branch November 13, 2023 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to make the reminder button configurable #430

Refactor to make the reminder button configurable #430

KaliedaRik commented Oct 27, 2023 •

edited

Loading

tjmw left a comment

tjmw Nov 7, 2023

KaliedaRik Nov 8, 2023

KaliedaRik Nov 9, 2023

tjmw Nov 7, 2023

KaliedaRik Nov 8, 2023

tjmw Nov 8, 2023

KaliedaRik Nov 9, 2023

tjmw Nov 7, 2023

KaliedaRik Nov 8, 2023

KaliedaRik Nov 9, 2023

tjmw Nov 7, 2023

KaliedaRik Nov 9, 2023

tjmw left a comment

tjmw Nov 10, 2023

tjmw Nov 10, 2023

tjmw Nov 10, 2023

KaliedaRik Nov 13, 2023

tjmw Nov 10, 2023

KaliedaRik Nov 13, 2023

tjmw Nov 10, 2023

KaliedaRik Nov 13, 2023

tjmw Nov 10, 2023

		interface ReminderCtaButtonProps {
		reminderComponent: ReminderComponent;


		type ColorStylesType = keyof AllAvailableColorStyles;

		export function getColors(userVals: Extras, defaults: Partial<AllAvailableColorStyles>) {

Refactor to make the reminder button configurable #430

Refactor to make the reminder button configurable #430

Conversation

KaliedaRik commented Oct 27, 2023 • edited Loading

What does this change?

How to test

tjmw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjmw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaliedaRik commented Oct 27, 2023 •

edited

Loading