feat!: Update behaviour of Header and related components #1607

VincentSmedinga · 2024-09-27T22:26:02Z

This implements the updated design for the Header.

It’s a significant PR that contains breaking changes for various components. It might be best to review this PR globally for the general approach and then extract smaller, focused PRs. This makes the detailed reviews more pleasant, updates the change log correctly, and helps our users understand and implement the changes.

Review my comment at the bottom to see the three approaches I considered and how I chose the winner.

UX agrees with the changes this PR introduces to the design – most notably, the generous white space – and will update Figma accordingly.

The big chunks that could be the new PRs:

Allow Grid and Grid Cell to render more tags
Allow the Logo to grow a bit
Introduce Item and Button for Page Menu
Implement new Header design
Use the new Header in Home Page example

These things remain to be done, preferably in separate PRs as well:

complete a11y – at least some accessible names are missing
render a narrow logo of just the St. Andrew’s Crosses
extract the AppHeader in the Amsterdam.nl home page to a pattern
test the updated Header design with the menus of existing sites
remove the Mega Menu component

Other tasks:

Include missing license headers (test files and some others)
Use HTMLElement and HTMLDivElement correctly
Decorate all stories with a Screen

VincentSmedinga · 2024-10-08T08:45:06Z

I’ve considered 3 approaches for the structure of HTML and components for the header, mega menu and search bar. For simplicity, the examples below do not contain the search bar, and only the menu button is specified for the Page Menu.

My requirements were:

Optimal HTML, accessibility and SEO
Use existing components where possible
A ‘natural’ API; not too many tricks

Approach 1: one explicit grid to contain everything

<Grid paddingVertical="medium">
  <Grid.Cell span="all">
    <Header menu={<PageMenu.Button>Menu</PageMenu.Button>} />
  </Grid.Cell>
  <nav className="ams-visually-hidden ams-display-contents">
    <Grid.Cell>
      <Columns>…</Columns>
    </Grid.Cell>
    <Grid.Cell>…</Grid.Cell>
  </nav>
</Grid>

+ One grid, no as props
+ Mega Menu is a pattern, not a component
+/- An independent component for CSS multi-columns
- Requires display: contents, unsure about browser support

Approach 2: two explicit grids

<Grid paddingVertical="medium">
  <Grid.Cell span="all">
    <Header menu={<PageMenu.Button>Menu</PageMenu.Button>} />
  </Grid.Cell>
</Grid>,
<Grid as="nav" className="ams-visually-hidden" paddingBottom="large">
  <Grid.Cell>
    <Columns>…</Columns>
  </Grid.Cell>
  <Grid.Cell>…</Grid.Cell>
</Grid>

+ Existing and logical API
+ Mega Menu is a pattern, not a component
+ Doesn’t need display: contents
+ Easy to increase bottom padding for menu alone
+/- An independent component for CSS multi-columns
- A bit more HTML (just one tag actually)

Approach 3: two implicit grids

<Header menu={<PageMenu.Button>Menu</PageMenu.Button>} />,
<MegaMenu className="ams-visually-hidden ams-display-contents" key={2}>
  <Grid.Cell>
    <Columns>…</Columns>
  </Grid.Cell>
  <Grid.Cell>…</Grid.Cell>
</MegaMenu>,

Header renders <Grid><Grid.cell span="all"><header class="ams-header" ref><Logo /><Heading />{menu}</header></Grid.Cell></Grid>

MegaMenu renders <Grid as="nav" className="ams-mega-menu" paddingBottom="large">{children}</Grid>

+ Simple outer API: Header and MegaMenu
+ MegaMenu.Section could implement CSS multi-col locally
- Two components render a Grid as well; dependencies, weird classes and refs
- Strange API: Grid.Cell as children of MegaMenu
- Requires display: contents, unsure about browser support

I chose approach 2.

storybook/src/components/MegaMenu/MegaMenu.stories.tsx

dlnr · 2024-10-08T09:39:37Z

storybook/src/pages/amsterdam/common/AppHeader.tsx

+  const toggleFeature = (feature: Feature) =>
+    setVisibleFeature((currentFeature) => (feature === currentFeature ? undefined : feature))
+
+  const visibilityClass = (feature: Feature) => (feature === visibleFeature ? null : 'ams-visually-hidden')


Is ams-visually-hidden the way to go? Yes you'll hide it but for the visually impaired, they will get the menu read out loud to them everytime. It should probably just use display: none and the menu (toggle) button should have aria-controls="mega-menu-id" so that the menu gets read to them when they toggle the menu.

Even Copilot agrees:

Using "visually hidden" to hide the megamenu may be a poor decision due to the following reasons:

Accessibility Issues: "Visually hidden" is intended for content that is hidden from sighted users but accessible to screen readers. Using it to hide interactive elements like a megamenu can confuse screen reader users as they might access the menu without visual context.

User Experience: Sighted users may face unexpected behavior if the megamenu is hidden visually but still interactable. This can lead to confusion and a poor user experience.

Best Practices: The repository guidelines suggest visually available content should also be accessible to non-visual user agents and vice versa. Hiding the megamenu only visually contradicts this practice.

Pretty cool it even refers to your own documentation

Correct, I haven’t implemented all accessibility features yet; I wanted to get the team’s feedback on this approach and API first. We will ensure that both sighted and non-sighted people can navigate properly. Screen reader users will want to hear the items, or they can use the Skip Link.

Someone suggested the Mega Menu must be in the DOM for search engines. That could well be – we should research that. Of course, we could employ other strategies like XML sitemaps, but I don’t expect they’ll allow us to hide the main menu for crawlers entirely.

I do remove the Search bar from the DOM, so I have considered it.

Pretty cool it even refers to your own documentation

Yes, it is!

Search engines execute javascript nowadays and are able to crawl the site without the help of the navigation. The child links of the Google search results are based on relevance, not the sitemap or main nav. Forcing visually impaired users to always use the skip link is not the way to go.

storybook/src/pages/amsterdam/common/AppHeader.tsx

dlnr · 2024-10-08T09:46:29Z

proprietary/tokens/src/components/ams/logo.tokens.json

      "emblem": { "color": { "value": "{ams.color.primary-red}" } },
+      "min-block-size": { "value": "2.5rem" },


This is a conflict with block-size which has a min size of 1rem.

The two are intended to cooperate: the minimum height remains ‘40px’ as it was, but the logo is allowed to grow to ‘56px’. The result is 33% top padding, 33% logo, and 33% bottom padding (vertically).

alimpens · 2024-10-11T15:09:23Z

packages/react/src/Header/Header.tsx

+          </Heading>
+        )}
+      </div>
+      <div className="ams-header__section">{menu}</div>


This should be a nav, I think. The main navigation section is the Page Menu and the Mega Menu together.

Yes, but I think it would be best to let PageMenu render a nav as its root tag. This would add that semantics to the footer menu as well.

I think the Page Menu and the Main Menu should be separate navigation sections. Screen reader users can then select which menu they want to explore.

I think the Page Menu and the Main Menu should be separate navigation sections

I think that's a little odd, semantically. The entire thing is our main navigation section imo, I don't really see why we should separate them. The mega menu is just a sub menu of the main menu.

It also goes against the whole 'use landmark roles sparingly' thing.

See also these examples, although the last 2 use the nav tag a bit less sparingly than I would:

https://www.gov.uk/

https://adobe-accessibility.github.io/Accessible-Mega-Menu/

https://design-system.w3.org/code/header-nav.html

https://www.rvo.nl/

alimpens · 2024-10-11T15:37:28Z

storybook/src/components/Header/Header.stories.tsx

  },
 }

-export const WithAppNameLinksAndMenuButton: Story = {
+export const WithAppNameMenuAndMenuButton: Story = {


Do we want to add a story here for the header + mega menu? It'd be nice to see the whole thing together, and I think I would look for it here.

I want to make that the first entry in a new ‘Patterns’ section. Header + Mega Menu is a composition with some state: a pattern.

(The confusion may be that our Header component is just Logo + Heading + Page Menu, while visually the Main Menu and the Grid whitespace around it would conceptually belongs to the (application) header as well. I considered renaming Header to Masthead or Navigation Bar or something.)

Another reason is that this will give us room to present a couple of variations for larger and smaller sites, based on the navigation of some of our current sites.

I think this depends on where we'll end up with our semantic definition of the Header. I think our Header contains our main navigation, which is comprised of the Page Menu and the Mega Menu. If we go with that definition, then the Mega Menu is part of the Header.

alimpens · 2024-10-11T15:52:05Z

storybook/src/pages/amsterdam/common/AppHeader.tsx

+  const toggleFeature = (feature: Feature) =>
+    setVisibleFeature((currentFeature) => (feature === currentFeature ? undefined : feature))
+
+  const visibilityClass = (feature: Feature) => (feature === visibleFeature ? undefined : 'ams-visually-hidden')


I just tested this with a screen reader, it's kinda of annoying that it always reads out the mega menu, even if it's collapsed. It also messes up your tab index. I think we should just use display: none here, same as gov.uk. Do you have a link to the comment where someone said this was a good idea?

The ‘SEO’ comment is kind of concise :)

Robbert has more on making header and navigation accessible.

Ha, that's a little hard to put into context. One thing I can comment on: Using display: none is not the same as not having the menu in the DOM. I agree it should be in the DOM, but visually hidden causes too many a11y issues imo.

VincentSmedinga · 2024-10-15T20:52:57Z

As suggested in the description, I’ve started extracting separate, more focused PRs.

I’m converting this PR to a draft. We may continue reviewing here, but let’s not merge it.

VincentSmedinga added 3 commits September 30, 2024 10:32

Simplify rigourously

b8bac3d

Change menu prop from ReactNode to callback function

e329c9a

Implement wrapping in two halves

359edc6

VincentSmedinga force-pushed the poc/DES-884-single-line-header branch from e4a4c98 to 359edc6 Compare September 30, 2024 08:32

github-actions bot temporarily deployed to demo-DES-884-single-line-header September 30, 2024 08:32 Destroyed

Wrap Header and Footer stories in Screen

285a869

github-actions bot temporarily deployed to demo-DES-884-single-line-header September 30, 2024 08:39 Destroyed

github-actions bot temporarily deployed to demo-DES-884-single-line-header September 30, 2024 09:28 Destroyed

VincentSmedinga force-pushed the poc/DES-884-single-line-header branch from 99ce081 to 285a869 Compare September 30, 2024 09:38

github-actions bot temporarily deployed to demo-DES-884-single-line-header September 30, 2024 09:39 Destroyed

VincentSmedinga added 9 commits September 30, 2024 12:27

Introduce component for buttons in page menu

e4f6df5

Use Page Menu in Header (incorrectly for now)

73e88f9

Allow icon in Page Menu Button

29bb49f

Allow Grid to render a different HTML element

834e72c

Make Header render its own Grid

c6efcef

Make Leader render its children

e6e5cf2

Include menu button in Page Menu

1a28c0c

Rename links prop to menu

7fa2903

Update homepage example

0615ff6

github-actions bot temporarily deployed to demo-DES-884-single-line-header September 30, 2024 12:08 Destroyed

VincentSmedinga added 3 commits September 30, 2024 16:39

Make Grid a decorator in Mega Menu stories for easier reuse

9f69f31

Simplify white space in Header

c8f8f91

Allow logo to grow a bit in wide windows

ca7fa54

github-actions bot temporarily deployed to demo-DES-884-single-line-header September 30, 2024 14:41 Destroyed

Hide secondary page menu items in narrow windows

6432adc

github-actions bot temporarily deployed to demo-DES-884-single-line-header September 30, 2024 18:53 Destroyed

VincentSmedinga added 2 commits September 30, 2024 21:03

Improve spacing in page menu

5f547ec

Merge branch 'develop' into poc/DES-884-single-line-header

63b43c7

github-actions bot temporarily deployed to demo-DES-884-single-line-header September 30, 2024 19:14 Destroyed

Remove obsolete class name

9e9fd4b

VincentSmedinga added 2 commits October 8, 2024 10:20

Implement Mega Menu as pattern in Amsterdam home page

b62fd4c

Merge branch 'develop' into poc/DES-884-single-line-header

b379529

github-actions bot temporarily deployed to demo-DES-884-single-line-header October 8, 2024 08:21 Destroyed

dlnr reviewed Oct 8, 2024

View reviewed changes

storybook/src/components/MegaMenu/MegaMenu.stories.tsx Show resolved Hide resolved

dlnr reviewed Oct 8, 2024

View reviewed changes

VincentSmedinga added 2 commits October 8, 2024 14:31

Fix empty class name

8ecf069

Remove unnecessary keys from example

e6cdd94

github-actions bot temporarily deployed to demo-DES-884-single-line-header October 8, 2024 14:04 Destroyed

alimpens reviewed Oct 11, 2024

View reviewed changes

This was referenced Oct 15, 2024

feat: Let Grid and Grid Cell render any structural tag #1662

Merged

feat!: Introduce Page Menu Item and Button #1663

Open

VincentSmedinga marked this pull request as draft October 15, 2024 20:53

alimpens added 2 commits October 23, 2024 12:15

Implement hack to wrap all main navigation in one nav element

9657743

Finetune poc

6891717

github-actions bot temporarily deployed to demo-DES-884-single-line-header October 23, 2024 13:17 Destroyed

alimpens added 2 commits October 23, 2024 15:40

Add padding

3e20118

Add toggle button

5685c69

github-actions bot temporarily deployed to demo-DES-884-single-line-header October 23, 2024 14:06 Destroyed

alimpens added 2 commits November 1, 2024 14:11

Break up menu in subcomponents

0101037

Remove unused state

fbfd1c1

github-actions bot temporarily deployed to demo-DES-884-single-line-header November 1, 2024 13:29 Destroyed

alimpens added 3 commits November 1, 2024 15:55

Merge item and link

10511d5

Move styling from inline to css

dd1c504

Cleanup, add MegaMenuSecondaryLinkList

b5df56d

github-actions bot temporarily deployed to demo-DES-884-single-line-header November 1, 2024 15:22 Destroyed

alimpens added 2 commits November 2, 2024 11:44

Add minimal version for headers without mega menu

9d595cd

Fix stories

a16e9af

github-actions bot deployed to demo-DES-884-single-line-header November 4, 2024 08:38 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Update behaviour of Header and related components #1607

feat!: Update behaviour of Header and related components #1607

VincentSmedinga commented Sep 27, 2024 •

edited

Loading

VincentSmedinga commented Oct 8, 2024

dlnr Oct 8, 2024

dlnr Oct 8, 2024

dlnr Oct 8, 2024

VincentSmedinga Oct 8, 2024

dlnr Oct 22, 2024

dlnr Oct 8, 2024

VincentSmedinga Oct 8, 2024

alimpens Oct 11, 2024

VincentSmedinga Oct 11, 2024

alimpens Oct 14, 2024

alimpens Oct 11, 2024

VincentSmedinga Oct 11, 2024

alimpens Oct 14, 2024

alimpens Oct 11, 2024

VincentSmedinga Oct 11, 2024

alimpens Oct 14, 2024

VincentSmedinga commented Oct 15, 2024 •

edited

Loading

		"emblem": { "color": { "value": "{ams.color.primary-red}" } },
		"min-block-size": { "value": "2.5rem" },

feat!: Update behaviour of Header and related components #1607

Are you sure you want to change the base?

feat!: Update behaviour of Header and related components #1607

Conversation

VincentSmedinga commented Sep 27, 2024 • edited Loading

VincentSmedinga commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentSmedinga commented Oct 15, 2024 • edited Loading

VincentSmedinga commented Sep 27, 2024 •

edited

Loading

VincentSmedinga commented Oct 15, 2024 •

edited

Loading