refactor: re-write header menu bar, remove bulma

[emilyjablonski]

Pull Request Template

Issue

Addresses #1345 #1467

• This change addresses the issue in full
• This change addresses only certain aspects of the issue
• This change is a dependency for another issue
• This change has a dependency from another issue

Description

We had been using Bulma as a dependency for our Site Header. We had four main issues with this, which were that customizing the header bar was becoming increasingly difficult as we had to override their styles and behavior, the Menu text was untranslated, some of the styles were just a little visually off, and the dropdown menu was not accessible on mobile.

This PR re-writes our SiteHeader from mostly scratch. There are new props to toggle certain visual choices on/off. I also refactored how we are sending in languages and the home page link to remove business logic. Also fixed a bug where header links were still showing in partners when signed-out.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Prototype/POC (not to merge)
• This change is a refactor/address technical debt
• This change requires a documentation update
• This change requires a SQL Script

How Can This Be Tested/Reviewed?

Check out the new stories, however please note that Storybook is not accurately displaying the mobile view in its default viewing window. Would recommend starting locally or the deploy preview to view mobile.

In Bloom we are not flattening mobile menus, using the mobile menu button, using the mobile dropdown, and not showing the feedback bar on mobile.
In DAHLIA, they will be flattening mobile menus, using mobile text as a button, using the mobile drawer, and showing the feedback bar on mobile.

These are all toggleable features through props.

I would test:

• Accessibility in desktop and mobile (can you navigate through all things in the header and dropdowns using your keyboard, are they all focusable and actionable on enter)
• Changing the window sizes to view the transition between desktop and mobile
• Comparing visually this PR to https://dev-bloom.netlify.app/ and https://partners.bloom.exygy.dev/ - there are some styling differences which are purposeful based off of the designs here https://www.figma.com/file/tUtgsJO8XnF9Ij2dwu22EF/bloom-general?node-id=387%3A768

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have reviewed the changes in a desktop view
• I have reviewed the changes in a mobile view
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules
• I have assigned reviewers
• I have updated the changelog to include a description of my changes
• I have run yarn generate:client if I made backend changes

[netlify]

✔️ Deploy Preview for dev-bloom ready!

🔨 Explore the source changes: b7d4202

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-bloom/deploys/615de09f8a6dfd0008e023c3

😎 Browse the preview: https://deploy-preview-1885--dev-bloom.netlify.app

[netlify]

✔️ Deploy Preview for dev-partners-bloom ready!

🔨 Explore the source changes: b7d4202

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-partners-bloom/deploys/615de09f2922820008010aac

😎 Browse the preview: https://deploy-preview-1885--dev-partners-bloom.netlify.app

[netlify]

✔️ Deploy Preview for dev-storybook-bloom ready!

🔨 Explore the source changes: b7d4202

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-storybook-bloom/deploys/615de09f30b92200070e8d10

😎 Browse the preview: https://deploy-preview-1885--dev-storybook-bloom.netlify.app

[emilyjablonski]

@akegan EDIT: Fixed the issue :) Turns out Storybook's small mobile setting is (I think) smaller than any of the mobile options from the console, which is why it looked differently! Storybook's official mobile settings match local now. Also added a new story that represents all the DAHLIA settings. Here's the preview: https://deploy-preview-1885--dev-storybook-bloom.netlify.app/?path=/story/headers-site-header--basic

[akegan]

LGTM, just with one note. The close button for the mobile menu with our style needs an aria label

I don't really understand why the a11y issue shows up in storybook but isn't caught by the automated a11y tests. I wonder if you'd be up for taking a second to see why that might be too. Might be because the test doesn't open up the menu? If not I can also look into it.

Edit: I just checked and it doesn't show up in the storybook tab unless it's mobile size and the menu is open (and if you click back to actions then back to accessibility again.

I wonder if this is something that would be better caught by some static linting similar to what we have in DAHLIA

[emilyjablonski]

@akegan We have a ticket to set up the same a11y linting so hopefully that will help!

[akegan]

LGTM for DAHLIA!

[slowbot]

@emilyjablonski You can remove the padding from the preview with this setting
https://dev.to/ludder/remove-padding-from-storybook-5ad0

[slowbot]

1. It seems we could use some padding around the text based logo on mobile
2. Can we add full height for mobile drawer menu

[emilyjablonski]

@slowbot we can add padding for sure! and the mobile drawer is full height there's just no left border and nothing for it to cover up (check dahlia prod for an example)

[slowbot]

@emilyjablonski Can we use rems instead of px whenever possible?

[emilyjablonski]

@slowbot For sure but I just did an overview of every case where I'm using px in the scss file and I can't change any of them to rem bc they need to be exact numbers for positioning

[slowbot]

LGTM

[jaredcwhite]

I'd like to discuss how to adjust all the navbar links so they're a tags and remove most of the click handlers and Next router statements. Unless I'm missing something, most of the links are indeed hrefs to other pages, so it's not semantic to use click handlers to navigate to a new page. We can also make use of NavigationContext/LinkComponent to ensure proper routing using the current Next locale.

[jaredcwhite]

Looking at the public website:

Semantically I would expect these all to be a tags, unless it's something that only performs an action in-page (aka no href), in which case it should be a button tag just styled to look like a link.

The generated HTML seems to be just div/span tags which wouldn't behave like a link or a button:

[emilyjablonski]

@jaredcwhite EDIT: After touching base offline, moving to a tags where I can, but not sure yet how adding localization pieces will work for DAHLIA since the current siteheader is broken for them on localization, gonna touch base w the DAHLIA team

[emilyjablonski]

@jaredcwhite Okidokie I made some updates. I'm not able to blanket use LinkComponent because some of the "links" in the SiteHeader actually just have onClicks and no href, so I'm toggling LinkComponent or button on and off based on what was passed in, and there are no more vanilla a tags. I had to modify NavigationContext to get it to fully work which I think was the whole initial issue DAHLIA was having with localization. We need to spread on all props in order to pass in some of what we need for the SiteHeader and for consumers to localize.

The LinkComponent is my biggest remaining area of concern still when it comes to making consumers use the NavigationContext and LinkComponent, because if DAHLIA (or any other consumer) sets up their LinkComponent and its props incorrectly, any component we have that uses LinkComponent won't even build for them, so we're kind of relying on them to set theirs up and I still think we should have them pass in a pre-localized href. But I think if we want to do that at all that should be a separate issue considering the priority of this PR so it doesn't matter atm anyway :)

[jaredcwhite]

@emilyjablonski Approach looks good. Let's continue to discuss ways we can make the link component injection easier to understand for consumers after this PR is merged.

One thing I noticed (shouldn't hold up the merge): the classes in the HTML have extra undefined…probably just missing array values somewhere in the component chain that are getting mapped to the string as undefined.