Skip to content

pkp/immersion#126 Navigation landmark missing label#158

Open
israelcefrin wants to merge 2 commits into
pkp:mainfrom
israelcefrin:i126_navigation_landmark_missing_label
Open

pkp/immersion#126 Navigation landmark missing label#158
israelcefrin wants to merge 2 commits into
pkp:mainfrom
israelcefrin:i126_navigation_landmark_missing_label

Conversation

@israelcefrin

Copy link
Copy Markdown
Contributor

Add aria-label for the landmark nav components on the top.

Issue: #126

@israelcefrin

Copy link
Copy Markdown
Contributor Author

Hi @kaitlinnewson ,

This PR fixes the missing label for navigation menus: #126

@kaitlinnewson

Copy link
Copy Markdown
Member

Hi @israelcefrin, sorry for the delay on reviewing this!

I think it looks fine, but wondered if it makes sense to use an aria-labelledby instead, and point to the respective h2 elements inside these navs which are using the same label?

@israelcefrin

Copy link
Copy Markdown
Contributor Author

Hi @kaitlinnewson

My apologies for my delay, I will test the aria-labelledby to use the current H2 text labels.

@israelcefrin

Copy link
Copy Markdown
Contributor Author

Hi @kaitlinnewson ,

I've switched it to arialabelledby and the code works and is much more simple. I've had to add IDs to each heading.

@kaitlinnewson kaitlinnewson linked an issue Apr 14, 2026 that may be closed by this pull request
id="immersion_content_header"{if $immersionHomepageImage} style="background-image: url('{$publicFilesDir}/{$immersionHomepageImage.uploadName|escape:"url"}')"{/if}>
<div class="container-fluid">
<nav class="main-header__admin{if $localeShow} locale-enabled{else} locale-disabled{/if}">
<nav class="main-header__admin{if $localeShow} locale-enabled{else} locale-disabled{/if}" aria-labelledby="immersion_content_header_user_menu">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the h2 below is wrapped in {if !empty(trim($userMenu))}, should this also be to ensure it exists?

@kaitlinnewson

Copy link
Copy Markdown
Member

Hi @israelcefrin, just one additional comment on this one - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[A11Y] Navigation landmark missing label

2 participants