fix(banner-notification): type check error on docs template

[Mousticke]

Description

The boolean passed from docs template to banner notification component
has an issue when the variable passed is null. The interface of the
banner props has shouldShow declared as boolean or undefined. To prevent
each component to check against null to boolean, the check is made
internally inside the banner component and shouldShow is declared as
boolean or null or undefined.

Related Issue

Related to : #6392

[gatsby-cloud]

Gatsby Cloud Build Report

ethereum-org-website-dev

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 19m

Performance

Lighthouse report

Metric	Score
Performance	🔶 21
Accessibility	💚 91
Best Practices	💚 100
SEO	💚 92

🔗 View full report

[pettinarip]

Hey @Mousticke thanks for catching this issue. I don't think the problem is in the BannerNotification component.

BannerNotification is a UI dumb component, it is agnostic of the consumers logic or problems. The problem is on the consumer side, the consumer (template/docs.tsx in this case) should send the correct value (a boolean) to the component.

// template/docs.tsx
const isPageIncomplete = !!mdx.frontmatter.incomplete // force it to be a boolean

// now we are always sending a boolean to the component
<BannerNotification shouldShow={isPageIncomplete}>

So, if you agree, we should revert these changes and fix it in the docs.tsx template.

[Mousticke]

@pettinarip It was my first attempt to do something like this

const isPageIncomplete = !!mdx.frontmatter.incomplete // force it to be a boolean

Here was my thought :
I realized that if another component imports the BannerNotificaton component and set the shouldShow value with another variable, then the type guard has to be made again and again. In this way, I was saying myself that I can factor inside the component itself. As you said, the BannerNotification is agnostic. So the props shouldShow (if provided) has to work with false,true, undefined and null values.

If you do not agree, let's revert and fix it in docs.tsx :)

[pettinarip]

Thanks for sharing your thought @Mousticke.

The problem with checking null is that you are fixing a problem that is not a concern of the component. Is not its responsibility to check that. null is a different value type.
We are defining an interface to force the consumers to respect that. E.g. if the consumer sends an empty string, the component shouldn't have a check to account for that case. I don't like adding more logic to the component to fix the problems of the consumers.

[Mousticke]

@pettinarip Understood and It makes sense.
Let's revert :)

[pettinarip]

Thanks to you, for your comprehension and the time you put into this 💪🏼

[pettinarip]

Thanks @Mousticke 🚀