Skip to content
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

Moved `<footer>`outside of `<div role='main'>` #2316

Closed
wants to merge 1 commit into from

Conversation

@sukiletxe
Copy link
Contributor

sukiletxe commented Apr 9, 2016

This enhances accessibility, particularly because a footer is not part of
the main content of the page.
Fixes #2309

By the way, I didn't Jinjify the result, because I tried, and a lot of whitespace changes (evil ^m on Windows) apeared everywhere as a result. So I thought you'd prefer a cleaner diff.

This enhances accessibility, particularly because a footer is not part of
the main content of the page.
Fixes #2309
@Kwpolska
Copy link
Member

Kwpolska commented Apr 9, 2016

Bootstrap itself mixes it in their examples… would you mind complaining there first?

@tritium21
Copy link
Contributor

tritium21 commented Apr 11, 2016

The test failure is in the invariant build. The test should be updated with this PR.

The test is checking the html output, and comparing it to a per-modification version; you just have to make the same change to the test that you made to the templates.

@Kwpolska
Copy link
Member

Kwpolska commented Apr 11, 2016

The test failure cannot be averted by contributors and needs to be handled by a core dev (who has access to the invariant-builds repo, which introduces a race-condition)

@tritium21
Copy link
Contributor

tritium21 commented Apr 11, 2016

Oh. I didn't know that part. At least it is documented why the one travis build is failing then.

@sukiletxe
Copy link
Contributor Author

sukiletxe commented Apr 13, 2016

Okay, so I have filed an issue, and they are willing to fix it. The
problem is, which examples should I point out to them? I downloaded the
latest release and I've looked at the examples (docs / examples), and I
can see none with this behaviour, nor with a <main> and a <footer>.
Perhaps you meant a theme?

On 09/04/2016 16:21, Chris Warrick wrote:

Bootstrap itself mixes it in their examples… would you mind
complaining there first?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#2316 (comment)

@Kwpolska Kwpolska added the visual label Apr 13, 2016
@Kwpolska
Copy link
Member

Kwpolska commented Apr 13, 2016

footer within div#container: https://getbootstrap.com/examples/jumbotron/ or https://getbootstrap.com/examples/carousel/

footer outside of div#container: https://getbootstrap.com/examples/blog/ or https://getbootstrap.com/examples/sticky-footer/

Looking closer, it might just be intentional for styling and not for accessibility.

If you moved footer outside of .container, you would ruin its appearance. This patch moves it from .row up to div.body-content, which does not change anything (especially role="main"). That said, I’m afraid I have to close this PR…

But thanks for caring!

@Kwpolska Kwpolska closed this Apr 13, 2016
@Kwpolska Kwpolska added the wontfix label Apr 13, 2016
@sukiletxe
Copy link
Contributor Author

sukiletxe commented Apr 14, 2016

OK, thanks for the info.
Regarding your second comment: perhaps an extra <div> could be created
(which encloses the <div>with role="main"), and the role could be
set for the inside <div>. Then, the inner <div> could be closed and
the <footer> could go after it. This would solve the issue, and I
think it wouldn't affect styling.

@sukiletxe
Copy link
Contributor Author

sukiletxe commented Jun 5, 2017

Looking through old issues, I see that this was rejected in Bootstrap's side (see twbs/bootstrap#19730). So... May I ask you to reconsider this? Thank you very much.

@Kwpolska
Copy link
Member

Kwpolska commented Jun 6, 2017

If they won’t fix it, we also have no reason to. Fix it in your custom templates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.