CAR-4260 extract header

[lkappeler]

• Extracts the header in three theme variations, LIGHT, DARK and TRANSPARENT.
• Provides a HeaderDropdown component to add navigation items with sub navigations (eg language selection) for desktop and mobile
• Extracts related items: Profile, Plus, MyCars, Logo, Logout, Heart

All Navigation items are passed dynamically to the component by the project using the component; See examples.

Walkthrough: https://drive.google.com/file/d/1AifAmq--GkANd5uKLSnyMeHjap9OHv-F/view?usp=sharing

Designs: https://app.abstract.com/share/c8774e1e-b5d2-467f-85d4-e3d13000af3d
Refs: https://autoricardo.atlassian.net/browse/CAR-4199

---

Inputs Enrique:

• use the bigger logo for lg and center the links in case they use two rows
• confirmed smaller cta button for md

---

Inputs Christian:

• Make the logo static linking to carforyou.ch
• create variations of the logo instead of passing colours
• Simplify SVG's to current colour clean up tailwind config
• Remove icon export
• Button refactoring change prop to responsive
• Inline example icons storybook
• Render props: only for the profile, make it consisten with the dealerhub approach
• Move the language navigation to the compnent; Decided to keep it passed as there is state and project specific dependencies
• Remove confusing or combinations form the context

[Averethel]

Please add .DS_Store to your global .gitignore

[Averethel]

Really nice first step.
Is there a way you could think of using tailwind colours in icon components?
One thing worth trying is setting fill property to currentColor and wrapping it in a container with a colour class.

I consider direct use of hex values a blocker - we're using tailwind to abstract those and not need to worry about changing them in multiple places, putting them directly in the code defeats hat purpose.

[lkappeler]

@Averethel

I will change it to use the tailwind colours instead.

I consider direct use of hex values a blocker - we're using tailwind to abstract those and not need to worry about changing them in multiple places, putting them directly in the code defeats hat purpose.

Be aware that the existing icon components, using fill colours (chat and icon), are using hardcoded colours as well. (Did not check the listing and dealerhub project but I have in mind it's the same there...) So we gonna need to refactor those at some point if we want to be able to change the colors only in the tailwind config...

[ralucacril]

smaller comments. up to you!

[ralucacril]

can i test this somewhere? that would be awesome to see it 🙏

[lkappeler]

@ralucacril There is a video in the description, you can also check out the branch locally and got to http://localhost:6006/?path=/story/header--theme-light. In the carforyou demo content section, there is a case that's nod handled by the demo (I think for the md breakpoint, where) will ad an update as we will need it for the usage anyways

[GutierrezGo]

I think, we must call it large instead small.

[lkappeler]

Whoopsie 👍

[GutierrezGo]

I think, we must call it small instead large.

[GutierrezGo]

Try to change z-dropdownMenu= 20px to 30px. Then the dropdown won't be behind the filters.

[ralucacril]

if you consider changing the z-dropdownMenu, please take in consideration that other things might break. there was a huge mess some time ago with z-index in our projects and all the numbers have some logic there. thanks!

[lkappeler]

Created a separate z-index for the headerDropdowns to be sure it's not breaking any exsisting behaviours.

[ralucacril]

thank you!

[masone]

sorry for issuing a blocker from my side so late. I have quarrels with some of the approaches we took here and would like to spend an extra round polishing the approach 🙇

[masone]

great decision to move away from boolean props for these now that you have more than 2 versions

I'm sure you'll have it covered, but just so I did my part: this will be a breaking change :)

[masone]

I would probably have bundled this change in a separate PR even when you need it to achieve the header

the reasoning is that when you squash, you'll end up with a commit along the lines of:

feat: add header component

BREAKING CHANGE: `small` prop no longer available on `<Button>`, please use `size` instead

the breaking change is just a side effect of the header, but it will be hard to communicate that in a commit message

alternatively, we could have two commits in one PR. one for the header and one for the button, eg.

feat: add size auto to Button

BREAKING CHANGE: `small` prop no longer available on `<Button>`, please use `size` instead

feat: add header component

but that does not seem feasible at this point where you have so many commits to edit in the git history

[masone]

does that make sense? if not, let's talk about it!

no need to split at this point. just wanted to raise awareness for the future :)

[masone]

when seeing auto I interpreted it would be something about the width of the button to be width: auto. that's might really just be my mind... alternatives might be responsive or adaptive

no strong feelings here, just a comment

[masone]

I would probably have had two separate SVGs instead of making these configurable like that. I'm pretty early in the review, but at this point it feels a bit overengineered and makes you to jump through some hoops to provide the colors (eg. there's a new color in the tailwind config that is uniquely used for the logo, which could have just been in the SVG)

[masone]

is this minimized?

[lkappeler]

yes it has quiet large path, ran all the image trough svgo multiple times

[masone]

I would prefer to keep the styles out of the icon components. at some point we will start generating these icon components based on the SVG files ("see new approach to SVG" discussion we didn't have yet) and then all these custom props will not hold up anymore

I also find it a breach of "separation of concerns" that we require an SVG to be aware of the tailwind config

[masone]

this goes for most added SVG components...

[masone]

do you require these icons in the project? If not, please don't export them
please also note that we yet have not shared SVGs through components on purpose

if you remove these from the export because they're used only internally, I'm happy to move forward. if you expose these, I'd like to polish the components a bit before merging. reasoning is that any internal change is easy as it's just a refactoring within this repo. in contrast, changing the interface later will require us to update all projects there these are used

[masone]

this looks substantially complicated and might lead to impossible state if someone fucks up. for example, isLight and isDark could end up being both true for some weird reason

[lkappeler]

@masone Tackled your inputs as discused, please take another look :)

[masone]

[masone]

the fragments should not be required here

Suggested change

	return <>{isOpen ? renderOpenState() : renderClosedState()}</>
	return isOpen ? renderOpenState() : renderClosedState()

[masone]

what do we need this for? don't we have global rules that allow for all our BEM conventions?

[lkappeler]

No they don't cover modifiers .bg-header_transparent

[masone]

comments like these are a great pointer to extracting the code into functions where you can slap a name on it renderDesktopLinks()

[lkappeler]

Nice input, makes it a look easier to read with the named functions :)

[masone]

is this minified already? eg with https://jakearchibald.github.io/svgomg/

[masone]

we might get a workflow for that soon :)

[lkappeler]

yeah I run svgo on all svg bevore copy pasta them to the typescript file

[autoricardo-bot]

🎉 This PR is included in version 7.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀