-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: allow to include the navigation bar in a single line header #3539
base: main
Are you sure you want to change the base?
Conversation
|
Hi @gfellerph as suggested, here is the direct PR |
Resolution pattern discussion: thumbs up |
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing the extensive rework necessary for providing this option and also thinking about documentation. The PR generally looks good, there are some things however that need to be fixed before we can merge it.
class="row justify-content-center top-navigation m-0 d-md-flex" | ||
aria-label="navigation" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes screenreaders to announce "navigation navigation" as the <nav>
already prompts a (localized) announcement of navigation. When adding this statically, there is also no chance to localize it.
aria-label="navigation" |
@@ -2,32 +2,40 @@ | |||
<div class="nav-down"> | |||
<div class="intranet-header"> | |||
<div class="d-flex align-items-center flex-nowrap"> | |||
<div class="bg-logo d-sm-block"> | |||
<div id="logo" class="bg-logo d-sm-block"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this id added? There is a chance of this id already being in use by a project, which could cause hard to find errors. If this id is necessary, we should prefix it with post-
or an even more specific prefix. Otherwise, please remove it.
Same goes for the other instances where ids were added.
|
||
<nav | ||
*ngIf="condenseHeader && hasNavbar" | ||
aria-label="navigation" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes screenreaders to announce "navigation navigation" as the <nav>
already prompts a (localized) announcement of navigation. When adding this statically, there is also no chance to localize it.
aria-label="navigation" |
private getAvailableNavWidth(): number { | ||
if (!this.condenseHeader) { | ||
return document.documentElement.scrollWidth; | ||
} | ||
|
||
const elements = [ | ||
this.logoElement, | ||
this.titleElement, | ||
this.optionHeaderContentElement, | ||
this.profileMenuElement, | ||
this.intranetSearchElement, | ||
]; | ||
const totalWidth = elements.reduce((sum, element) => sum + (element?.scrollWidth || 0), 0); | ||
|
||
return document.documentElement.scrollWidth - totalWidth; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice for @swisspost/design-system-maintainers, this PR needs to go to main and then needs a cherry pick to |
Allow the option to include the navigation bar in a single line as a condensed header. Instead of 2 lines only one (the top one) will contains the logo, title and navigation bar, as well as the settings. As discussed with Debray Alize and Gfeller Philipp.
Team NES
Langhard Matthias
Pedroso Yohandris