Skip to content

Conversation

@DerekNonGeneric DerekNonGeneric force-pushed the fix/missing-footer branch 3 times, most recently from d6f2fdb to 6a561b8 Compare July 15, 2020 04:08
@DerekNonGeneric DerekNonGeneric marked this pull request as draft July 15, 2020 08:18
@DerekNonGeneric DerekNonGeneric force-pushed the fix/missing-footer branch 4 times, most recently from 5babe6f to dd75cfe Compare July 16, 2020 10:46
@DerekNonGeneric DerekNonGeneric changed the title fix #5,#192, add proper footer fix #192, add missing footer Jul 16, 2020
@DerekNonGeneric DerekNonGeneric marked this pull request as ready for review July 16, 2020 13:46
@DerekNonGeneric
Copy link
Contributor Author

DerekNonGeneric commented Jul 16, 2020

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.

/cc @codehag @ljharb

@DerekNonGeneric DerekNonGeneric force-pushed the fix/missing-footer branch 3 times, most recently from 4b5efea to 5f7c900 Compare July 16, 2020 22:09
Copy link
Collaborator

@codehag codehag left a 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

Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

@codehag codehag Jul 27, 2020

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.

Copy link
Contributor Author

@DerekNonGeneric DerekNonGeneric Jul 27, 2020

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.

Copy link
Collaborator

@codehag codehag Jul 28, 2020

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sgtm

@DerekNonGeneric DerekNonGeneric force-pushed the fix/missing-footer branch 3 times, most recently from cc6e475 to 8da9ee5 Compare July 19, 2020 10:36
@DerekNonGeneric DerekNonGeneric marked this pull request as draft July 20, 2020 08:21
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
@DerekNonGeneric DerekNonGeneric marked this pull request as ready for review July 28, 2020 12:17
@DerekNonGeneric
Copy link
Contributor Author

DerekNonGeneric commented Jul 28, 2020

Thanks for the reviews so far, I think all feedback has been incorporated. 😄

@codehag codehag merged commit 211b701 into tc39:master Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Copyright Notice Missing From Footer

2 participants