-
Notifications
You must be signed in to change notification settings - Fork 35
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(eu-header): apply new specs - INNO-1203 #811
Conversation
Deploy preview for europa-component-library ready! Built with commit e7a1f91 https://deploy-preview-811--europa-component-library.netlify.com |
|
||
<SearchForm | ||
className="ecl-site-header__search" | ||
buttonLabel={searchForm.buttonLabel} |
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 not {...searchForm}
?
className="ecl-site-header__logo ecl-link ecl-link--standalone" | ||
href={logo.href} | ||
> | ||
<Logo title={logo.title} alt={logo.alt} language={logo.language} /> |
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 not <Logo {...logo} />
?
}); | ||
|
||
return ( | ||
<div className={classNames} {...props}> |
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 not use <header>
? and role="banner"
?
https://www.w3.org/TR/wai-aria-practices-1.1/examples/landmarks/banner.html
https://www.scottohara.me/blog/2018/03/03/landmarks.html
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.
</a> | ||
|
||
<div className="ecl-site-header__selector"> | ||
<a |
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 not use <Link ... />
?
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.
because Link do not allow anything else from simple text for now
@@ -25,8 +25,8 @@ const SearchForm = ({ textInputId, buttonLabel, className, ...props }) => { | |||
}; | |||
|
|||
return ( | |||
<form {...props} className={classNames}> | |||
<TextInput {...searchTextInput} /> | |||
<form {...props} className={classNames} role="search"> |
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.
Can you update the EC version as well? :)
PR description
Apply specifications of eu header
Also contains language selector
Note that language selector is just a link for now.
Overlay will be added once language list has been done (INNO-1186)