-
Notifications
You must be signed in to change notification settings - Fork 64
fix #192, add missing footer #201
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
Conversation
d6f2fdb
to
6a561b8
Compare
5babe6f
to
dd75cfe
Compare
I decided not to tackle #5 in this PR, but got pretty far along before realizing that it would probably be best to split them up. If these changes look good to go and are merged (you may need to squash it first), I'll open up my PR that fixes #5. Those changes are a good segue into talking about the information architecture, which may need to be re-evaluated to achieve the desired automation. |
4b5efea
to
5f7c900
Compare
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.
Looks good, a few nits
_layouts/default.html
Outdated
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.
should this be in the main section?
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.
It's a good question. I decided to mirror the placement of the pre-existing page_header.html
above it since it's only a single page for now. https://github.com/tc39/tc39.github.io/blob/master/_layouts/default.html#L20
For a multi-page site, I would go w/ more of a so-called “global footer” that would exist outside of the <main>
element, which would have started at default.html#L27
instead.
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.
I don't think that, because it is a single page, that the footer should be inside the main content. The purpose of the structure of <nav></nav><main></main><footer></footer>
isn't for single pages vs multi-page sites -- its for semantic html. This is used by accessibility technologies and also helps keep the site well organized -- regardless of if it is a multi page site or a single page site
You can read more about that here https://developer.mozilla.org/en-US/docs/Learn/Accessibility/HTML and here https://webflow.com/blog/html5-semantic-elements-and-webflow-the-essential-guide
Let's move the footer out of the main body and, if you like -- in a separate patch deal with the nav. Looking at the code -- the nav is unnecessarily nested. There is likely cleanup there. There will probably be issues in the styling due to this nesting. That might be a nice next bug.
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.
It's hard to read because it isn't indented, but the page header is currently nested within the <main>
element.
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.
I'm aware that the header, which contains the nav, is currently in the main section.
If you read my comment again, I proposed moving the nav in the next patch. It is nested inside of the header in a single element, which in turn is in the main section.
if you like -- in a separate patch deal with the nav. Looking at the code -- the nav is unnecessarily nested
Please move the footer out of the main section. There is no reason for it to be there.
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.
[…] in a separate patch deal with the nav.
Do you think it would alright if this was taken care of in 0504624?
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.
sgtm
cc6e475
to
8da9ee5
Compare
Notes: https://user-images.githubusercontent.com/17770407/87853458-340d4f00-c8d8-11ea-889b-b8c476f7c3cb.png https://user-images.githubusercontent.com/17770407/87853471-4ab3a600-c8d8-11ea-8484-71242942ec23.png https://www.color-hex.com/color/696969 https://webaim.org/resources/contrastchecker/?fcolor=696969&bcolor=fafafa&api Fixes: tc39#192
Some low-vision users cannot distinguish between colors, so we must use an alternative way of signifying that a link is present and may be clicked. The convention of underlined link text has been applied to this footer. Notes: https://www.w3.org/TR/low-vision-needs/#not-relying-on-color https://user-images.githubusercontent.com/17770407/87872057-92433c00-c983-11ea-9686-3ce3dfe72260.png
A navigation landmark role has been added to identify this site info section. Notes: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/Navigation_Role
8da9ee5
to
0504624
Compare
Thanks for the reviews so far, I think all feedback has been incorporated. 😄 |
Notes:
https://user-images.githubusercontent.com/17770407/87853458-340d4f00-c8d8-11ea-889b-b8c476f7c3cb.png
https://user-images.githubusercontent.com/17770407/87853471-4ab3a600-c8d8-11ea-8484-71242942ec23.png
https://www.color-hex.com/color/696969
https://webaim.org/resources/contrastchecker/?fcolor=696969&bcolor=fafafa&api
Fixes: #192