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

404.html: better content vertical alignment #1567

Closed
wants to merge 7 commits into from
Closed

404.html: better content vertical alignment #1567

wants to merge 7 commits into from

Conversation

EvgenyOrekhov
Copy link
Contributor

Content vertical alignment using display: table for <html> and display: table-cell for <body>.
That way one can modify the content and don't worry about readjusting the margin.

EvgenyOrekhov and others added 2 commits July 27, 2014 21:11
Content vertical alignment using display: table for <html>
and display: table-cell for <body>.
@drublic
Copy link
Member

drublic commented Jul 30, 2014

Thanks for this PR, @EvgenyOrekhov.
I've tested this in IE8 and other browsers we support and it works perfectly. We should reset display: table on html for mobile browsers too, I think. Everything else looks pretty perfect to me.
You could add a note about this in CHANGELOG.md.

@drublic drublic added this to the 5.0.0 milestone Jul 30, 2014
@EvgenyOrekhov
Copy link
Contributor Author

@drublic done

@EvgenyOrekhov
Copy link
Contributor Author

@drublic, did I mess something up? Sorry, I'm new to Git and GitHub.

@alrra
Copy link
Member

alrra commented Jul 31, 2014

did I mess something up? Sorry, I'm new to Git and GitHub.

@EvgenyOrekhov don't worry, if it's going to be merge, we'll handle it!

IMHO it looks better when the second line isn't so long (maybe add something like p{ ...; margin: auto; width: 280px; }?).

Note(s):

  • Don't know how useful this change is since the 404 is intended to be a placeholder page that users should replace with their own.
  • It will not "work" with IE6/7 since they don't support CSS Table displays (reason why display: table wasn't used in the first place + the original idea was to also support them) => v4 shouldn't get this

@EvgenyOrekhov
Copy link
Contributor Author

@alrra I set width to 310px to make sure the word "view" stays on the first line in mobile browsers.

I liked the new 404 page, so I kept using it as it is in my projects, but I kept struggling with proper vertical alignment when I localized the page, so I suggested some h5bp users would also benefit from this change.

P. S. Am I doing it right? I should commit, then git pull upstream master, then git push origin <topic-branch-name>, right? Not git pull --rebase?

@arthurvr
Copy link
Member

Don't know how useful this change is since the 404 is intended to be a placeholder page that users should replace with their own.

@alrra I do think some of the users never touch the 404 page layout. Some just change/translate the text. For those users it will be better if we use this technique for vertical centering. With the technique we use right now the vertical alignment will break for them.

@alrra
Copy link
Member

alrra commented Jul 31, 2014

I set width to 310px to make sure the word "view" stays on the first line in mobile browsers.

@EvgenyOrekhov thanks, will check as soon as I get home.

P. S. Am I doing it right? I should commit, then git pull upstream master, then git push origin , right? Not git pull --rebase?

@EvgenyOrekhov After you make your changes and staged them, you can just amend the commit (git commit --amend), and then just force push on your topic branch, in this case to 404-centrify (git push -f origin 404-centrify)

I highly recommend: Pro Git.

I liked the new 404 page, so I kept using it as it is in my projects, but I kept struggling with proper vertical alignment when I localized the page, so I suggested some h5bp users would also benefit from this change.

I do think some of the users never touch the 404 page layout. Some just change/translate the text. For those users it will be better if we use this technique for vertical centering. With the technique we use right now the vertical alignment will break for them.

@EvgenyOrekhov, @arthurvr good to know that people are actually building upon it!

@EvgenyOrekhov
Copy link
Contributor Author

@alrra thanks, I thought "do not rewrite public history" is a strict rule.

@alrra
Copy link
Member

alrra commented Jul 31, 2014

I thought "do not rewrite public history" is a strict rule.

@EvgenyOrekhov yes, when it comes to the master branch

@@ -33,11 +33,18 @@

p {
line-height: 1.2;
margin: auto;
width: 310px;
}

@media only screen and (max-width: 270px) {

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@alrra
Copy link
Member

alrra commented Jul 31, 2014

@EvgenyOrekhov played a little bit and ended by with this.

  • How it looks (some examples):

    Device/OS (Browser) Screenshot
    Mac / OS X (Firefox)
    PC / Windows XP (IE 6/7)
    (looks pretty decent)
    iPad / iOS (Safari)
    Samsung Galaxy S III / Android (Chrome)
  • The different in size compared to the old 404 is negligible

    • old 404:

      original size:         1242 B
      gzipped size:           574 B
      
    • new 404:

      original size:         1274 B
      gzipped size:           570 B
      

@Roope
Copy link

Roope commented Jul 31, 2014

Looking good!

@EvgenyOrekhov
Copy link
Contributor Author

@alrra nice, but it would be even nicer if the line break appeared right after "view", so it could be like

Sorry, but the page you were trying to view
does not exist.

Don't you think?

@alrra
Copy link
Member

alrra commented Jul 31, 2014

Note: I've improved the page a little bit more (my previous comment has been updated).

would be even nicer if the line break appeared right after "view"

@EvgenyOrekhov personally I find it nicer the way it is now. :)

@EvgenyOrekhov
Copy link
Contributor Author

@alrra, applied your changes.

alrra pushed a commit that referenced this pull request Jul 31, 2014
Since some of the users prefer to just modify the placeholder 404
page instead of replacing it with their own, this commit makes it
easier to do that by allowing them to modify the content without
worrying about readjusting the margins.

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

Note: The different in size compared to the old 404 is negligible:

 * old 404:

   original size:         1242 B
   gzipped size:           574 B

 * new 404:

   original size:         1274 B
   gzipped size:           570 B

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

Ref: #1567.
Close: #1567.
@alrra
Copy link
Member

alrra commented Jul 31, 2014

Merged in c3a72ff.

Thanks @EvgenyOrekhov and congrats on your first HTML5 Boilerplate contribution! 🎉 👏

@alrra alrra closed this Jul 31, 2014
@EvgenyOrekhov
Copy link
Contributor Author

Thanks everyone! You made a great first impression of GitHub community!

@EvgenyOrekhov EvgenyOrekhov deleted the 404-centrify branch August 2, 2014 08:04
alrra pushed a commit to h5bp/mobile-boilerplate that referenced this pull request Aug 23, 2014
Since some of the users prefer to just modify the placeholder 404
page instead of replacing it with their own, this commit makes it
easier to do that by allowing them to modify the content without
worrying about readjusting the margins.

Ref h5bp/html5-boilerplate#1567
Close #219
eleanor-byhook pushed a commit to eleanor-byhook/html5-boilerplate that referenced this pull request Feb 29, 2016
Since some of the users prefer to just modify the placeholder 404
page instead of replacing it with their own, this commit makes it
easier to do that by allowing them to modify the content without
worrying about readjusting the margins.

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

Note: The different in size compared to the old 404 is negligible:

 * old 404:

   original size:         1242 B
   gzipped size:           574 B

 * new 404:

   original size:         1274 B
   gzipped size:           570 B

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

Ref: h5bp/html5-boilerplate#1567.
Close: h5bp/html5-boilerplate#1567.
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.

5 participants