Enhancement/Issue 68 add anchor link to header logo #69
Conversation
✅ Deploy Preview for super-tapioca-5987ce ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
src/components/header/header.js
Outdated
| @@ -16,9 +16,11 @@ export default class Header extends HTMLElement { | |||
| this.attachShadow({ mode: "open" }); | |||
| this.shadowRoot.innerHTML = ` | |||
| <header> | |||
| <a href="/" title="Greenwood Logo"> | |||
There was a problem hiding this comment.
A couple thoughts here:
- For the
titleof this link, I think we might probably want it to say Greenwood home page instead - I think we might want to wrap the
<a>tag from inside the div (thus having the inline element(s) nested inside the block level element), e.g.<div class="logo-bar"> <a href="/" title="Greenwood home page"> ${greenwoodLogo} </a> </div>
There was a problem hiding this comment.
@thescientist13 sometimes, we wrap clickable elements like cards, buttons with hyperlinks like below screenshot:
Reference: https://developer.mozilla.org/en-US/curriculum/. Let me now if that's okay.
I will proceed with other changes. Thanks!
There was a problem hiding this comment.
Thanks for that, and yeah was more so just confirming that<a> tags can contain block level elements within them, and so per this article, seems like what you have here should be fine. 👍
src/components/header/header.spec.js
Outdated
|
|
||
| expect(anchor).to.not.equal(undefined); | ||
| expect(anchor.getAttribute("href")).to.equal("/"); | ||
| expect(logo).to.not.equal(undefined); |
There was a problem hiding this comment.
expect(logo).to.not.equal(undefined);We already test for the logo / <svg> in a preview it block, so I think instead of this line, let's test for a proper title attribute on the link instead (which would be good for a11y), e.g.
expect(anchor.getAttribute("title")).to.equal("Greenwood home page");There was a problem hiding this comment.
@thescientist13 Updated the files as per the suggestions.Thanks!
| <div class="logo-bar"> | ||
| ${greenwoodLogo} | ||
| </div> | ||
| <a href="/" title="Greenwood Home Page"> |
There was a problem hiding this comment.
Just made a small indentation tweak here
Related Issue
Resolves #68
Summary of Changes