Sticky header #466
Sticky header #466
Conversation
Deployed to Cloudflare Pages
|
|
I tested this and it works great, but I'm a bit concerned by the fact that the composition of the components is different on desktop and mobile. (Ie. the search is included in PageLayou on desktop, but in Header on mobile.) While I see why this might have been the most natural way to implement this now from a technical point of view, I am concerned that maintaining this in the future might be difficult and error prone, because there are too many variations. Wouldn't it be possible to avoid this fundamental difference between desktop and mobile, somehow? |
| @@ -9,7 +9,7 @@ import { useTranslation } from 'react-i18next' | |||
|
|
|||
| interface LogotypeProps { | |||
| color?: string | |||
| showText: boolean | |||
| showText?: true | false | undefined // If undefined: shows text on desktop | |||
There was a problem hiding this comment.
Do we need both the question mark and the undefined value? Seems a bit redundant...
There was a problem hiding this comment.
This needs a different solution
showText?: true | false | undefined // If undefined: shows text on desktop
...
export const Logotype: FC<LogotypeProps> = ({ color, showText = true }) => {
...
const showTypography = showText ?? !isMobilenever results in ?? !isMobile executing
There was a problem hiding this comment.
I guess we don't need the default value here: showText = true
Other than that, it should be fine....
|
Rebased and split commits |
I think since @buberdds is not around ATM, we should just merge this, now that it's approved. |
No description provided.