Info Card -> PROD-2321 Bug Hunt Intake

[mmattlin]

What's in this PR?

A component called InfoCard to display a block of information within a colored card.

• Through props, one can choose the color of the card (gray, green or yellow) and whether it is collapsible or not.
• This component is not yet in use because we haven't started setting up the Bug Hunt intake form, but see below for example how it looks being used in one of our other forms

Screenshots

[mmattlin]

Optional reviewers: @brooketopcoder @testflyjets

[brooketopcoder]

@mmattlin this looks great! just one super small comment

[brooketopcoder]

Enums are great when you want intellisense and shorthand for a string or number.

Types are better when we want to limit the possible values of something but don't necessarily care about what those values actually are.

In this case, the value "gray" doesn't really mean anything other than we're setting a color.

Also, I generally avoid using colors to describe properties and instead name them after what those colors are supposed to represent. So maybe it would be something like info / warn / success?

What do you think of:

export type InfoCardStyle = 'info' | 'success' | 'warn'

Also, the file should be named to match the object: InfoCardStyle.type.ts

[hentrymartin]

@brooketopcoder how about combining both type and enum?

enum InfoCardStyleTypes {
   info: 'info',
  success: 'success',
  warn: 'warn',
}

export type InfoCardStyle = keyof typeof InfoCardStyleTypes

This way we don't have to hardcode 'info'/'success' in the code and in future we can easily extend it for a new type.

[brooketopcoder]

@hentrymartin that's an interesting idea!

i am a bit confused about what hardcoding 'info'/'success' means. A type isn't hardcoded. You can use intellisense the same as an enum.

If you do a global search on export type vs export enum you'll see the vast majority of the enums are doing a translation of a short key to a string with some sort of formatting. And the types are just keys to switch off of.

[hentrymartin]

without the enum, when we use this component in another component then we will do the following,

<InfoCard style="info" />

If there is a enum to define the styles then we can maintain these strings in it. In this case the above code will be like this,

<InfoCard style={InfoCardStyleTypes.info} />

This way, it is easy to extend the styles to a different style in future. All we have to do is to add a new property in the enum. That will automatically add the new style as a new type since the enum defines the type like this,

export type InfoCardStyle = keyof typeof InfoCardStyleTypes

[mmattlin]

@hentrymartin In a similar fashion, maintaining the styles in a type would simply require adding the string to the type
e.g. export type InfoCardStyle = 'info' | 'success' | 'warn' | 'error'

I don't see a clear advantage of using one over the other
@brooketopcoder

[hentrymartin]

@brooketopcoder @mmattlin I think maybe I couldn't able to explain clearly what I want to say. I don't want to use only enum but want to combine enum with types like this.

enum InfoCardStyleTypes {
   info: 'info',
  success: 'success',
  warn: 'warn',
}

export type InfoCardStyle = keyof typeof InfoCardStyleTypes

interface InfoCardProps {
    children: ReactNode,
    style: InfoCardStyle
    defaultOpen: boolean,
    isCollapsible: boolean,
    title?: string,
}

If you closely look into the above piece of code, it combines both type and enum. Combining both type and enum together gives us the ability to maintain the constant values(in this case 'info', 'success' etc) in one place.

Based on your example screenshot above, let assume that we are going to use the InfoCard component in 20 different places with string 'info' as prop value,

<InfoCard style={'info'} />

Let's say that in future we want to change the string 'info' to 'info-new', in this case you need to change in 20 different places. Do you agree with this?

If we follow the way I propose, using 'info' as string can be avoided instead we can use the enum to pass the value('info') as prop like this,

<InfoCard style={InfoCardStyleTypes.info} />

In this case, we just have to change the value inside the enum in one place that will affect in all the 20 places where InfoCardStyleTypes.info is used.

I hope this is clear if not we can get on a short call to discuss this. Why I am stressing this is that having strings in lot of different places makes the code not maintainable in long run(based on my experience :D ).

[brooketopcoder]

@hentrymartin i totally see your point. :) The use case you describe is exactly the use case when you'd need an enum.

My point in this conversation, though, is that the value of these options is meaningless. Whether we name the option 'success' or 'style-3', it's just a code to let the dev know what they're selecting.

So in this scenario, there is no reason that we would ever actually need to change the value of the option.

If the intent was to make these styles correlate with global class names, then that would be a use for an enum. For example:

export enum Styles {
    success: 'green-dark',
    warn: 'red-light'
}

But I don't love that pattern b/c it unnecessarily obfuscates where the class name mapping happens.

[hentrymartin]

@brooketopcoder ok cool. My only concern is about having constant strings('info', 'success', 'warn' etc.,) being hardcoded(according to me, it is still hardcoding 😄) in lots of places. I still believe that the following piece of code,

enum InfoCardStyleTypes {
   info: 'info',
  success: 'success',
  warn: 'warn',
}

export type InfoCardStyle = keyof typeof InfoCardStyleTypes

is equivalent to,

export type InfoCardStyle = 'info' | 'success' | 'warn'

only difference being in the first block of code, the type is generated based on the enum key value pair and holds the constants in one central place.

Anyhow, I can also see the point that these types will not be changed in future 👍🏻 so all good.

[mmattlin]

@hentrymartin @brooketopcoder I've changed the code back to my original design, which specified the color type as part of the props interface for InfoCard without adding any extra files which I thing is overkill for this scenario:

interface InfoCardProps {
    children: ReactNode,
    color: 'info' | 'success' | 'warn',
    defaultOpen: boolean,
    isCollapsible: boolean,
    title?: string,
}

[brooketopcoder]

@hentrymartin i hear ya. there are def trade-offs. and what's funny is that there are plenty of examples of me breaking my own pattern and using an enum where i should have used a type. so obviously they are pretty much equivalent.