Skip to content

Improve display of 404 pages #572

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 17, 2016
Merged

Improve display of 404 pages #572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions app/controllers/pageflow/application_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,14 @@ class ApplicationController < ActionController::Base

rescue_from ActiveRecord::RecordNotFound do
respond_to do |format|
format.html { render :file => Rails.root.join('public/404.html'), :status => :not_found }
format.any(:json, :css) { head :not_found }
format.html do
begin
render file: Rails.public_path.join('pageflow', 'error_pages', '404.html'), status: :not_found
Copy link

Choose a reason for hiding this comment

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

Line is too long. [106/100]

Copy link
Member

Choose a reason for hiding this comment

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

I'm still unsure if we should have a test for this? The line itself is covered. We could add an expection to the EntriesController#show spec that some text from the generated file shows up. For the rescue clause below, I see no way to get test coverage. A test could temporarily rename the 404.html file in the dummy app. But I'm not sure it's worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's worth it because the head :not_found in ApplicationController is part of our code. I added 527f703 to test this. It might not look pretty but it is effective. I added the safeguard since the test could fail for other reasons and leave the dummy app in an unclean state.

An alternative could be to not bother with file-level stuff (which technically doesn't belong in a controller test anyway) but stub out the call to render and raise ActionView::MissingTemplate from the test. But then you're testing Rails framework detail which might change at some point...

Not sure what we should prefer here. But I do think a test has value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as testing response content, that has little additional value to me over the status code. Also it makes the test brittle in case someone wants to put in new text in this file.

rescue ActionView::MissingTemplate
head :not_found
end
end
format.any { head :not_found }
end
end

Expand Down
3 changes: 0 additions & 3 deletions app/controllers/pageflow/entries_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,6 @@ def show
@entry = DraftEntry.find(params[:id])
authorize!(:show, @entry.to_model)
end
format.any do
render(:file => 'public/pageflow/404.html', :status => :not_found)
end
end
end

Expand Down
27 changes: 27 additions & 0 deletions lib/generators/pageflow/error_pages/error_pages_generator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
require 'rails/generators'

module Pageflow
module Generators
class ErrorPagesGenerator < Rails::Generators::Base
desc 'Generate error pages.'

source_root File.expand_path('../templates', __FILE__)

def create_not_found
copy_file '404.html', 'public/pageflow/error_pages/404.html'
end

def copy_fonts
directory 'fonts', 'public/pageflow/error_pages/fonts'
end

def copy_stylesheets
directory 'stylesheets', 'public/pageflow/error_pages/stylesheets'
end

def copy_images
directory 'images', 'public/pageflow/error_pages/images'
end
end
end
end
15 changes: 15 additions & 0 deletions lib/generators/pageflow/error_pages/templates/404.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8"/>
<title>Not Found (404)</title>
<link rel="stylesheet" type="text/css" href="/pageflow/error_pages/stylesheets/main.css" />
</head>

<body>
<div>
<h1>Not Found</h1>
<p>The page you've requested cannot be found.</p>
</div>
</body>
</html>
0 ...sets/fonts/sourcesanspro-bold-webfont.eot → ...ates/fonts/sourcesanspro-bold-webfont.eot
View file
Open in desktop
File renamed without changes.
0 ...sets/fonts/sourcesanspro-bold-webfont.svg → ...ates/fonts/sourcesanspro-bold-webfont.svg
View file
Open in desktop
File renamed without changes
0 ...sets/fonts/sourcesanspro-bold-webfont.ttf → ...ates/fonts/sourcesanspro-bold-webfont.ttf
View file
Open in desktop
File renamed without changes.
0 ...ets/fonts/sourcesanspro-bold-webfont.woff → ...tes/fonts/sourcesanspro-bold-webfont.woff
View file
Open in desktop
File renamed without changes.
0 ...s/fonts/sourcesanspro-regular-webfont.eot → ...s/fonts/sourcesanspro-regular-webfont.eot
View file
Open in desktop
File renamed without changes.
0 ...s/fonts/sourcesanspro-regular-webfont.svg → ...s/fonts/sourcesanspro-regular-webfont.svg
View file
Open in desktop
File renamed without changes
0 ...s/fonts/sourcesanspro-regular-webfont.ttf → ...s/fonts/sourcesanspro-regular-webfont.ttf
View file
Open in desktop
File renamed without changes.
0 .../fonts/sourcesanspro-regular-webfont.woff → .../fonts/sourcesanspro-regular-webfont.woff
View file
Open in desktop
File renamed without changes.
0 public/pageflow/error_assets/images/bg.jpg → ...eflow/error_pages/templates/images/bg.jpg
View file
Open in desktop
File renamed without changes
40 changes: 40 additions & 0 deletions lib/generators/pageflow/error_pages/templates/stylesheets/main.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
@font-face {
font-family: SourceSansPro;
font-weight: normal;
font-style: normal;
src: url(/pageflow/error_pages/fonts/sourcesanspro-regular-webfont.eot);
src: url(/pageflow/error_pages/fonts/sourcesanspro-regular-webfont.eot?#iefix) format("embedded-opentype"), url(/pageflow/error_pages/fonts/sourcesanspro-regular-webfont.woff) format("woff"), url(/pageflow/error_pages/fonts/sourcesanspro-regular-webfont.ttf) format("truetype"), url(/pageflow/error_pages/fonts/sourcesanspro-regular-webfont.svg#SourceSansPro) format("svg");
}

@font-face {
font-family: SourceSansPro;
font-weight: bold;
font-style: normal;
src: url(/pageflow/error_pages/fonts/sourcesanspro-bold-webfont.eot);
src: url(/pageflow/error_pages/fonts/sourcesanspro-bold-webfont.eot?#iefix) format("embedded-opentype"), url(/pageflow/error_pages/fonts/sourcesanspro-bold-webfont.woff) format("woff"), url(/pageflow/error_pages/fonts/sourcesanspro-bold-webfont.ttf) format("truetype"), url(/pageflow/error_pages/fonts/sourcesanspro-bold-webfont.svg#SourceSansPro) format("svg");
}

html {
background-color: #252526;
background-image: url('/pageflow/error_pages/images/bg.jpg');
background-repeat: no-repeat;
background-position: center center;
height: 100%;
}

body {
font-size: 21px;
color: #fff;
font-family: "SourceSansPro";
}

div {
position: absolute;
top: 30%;
left: 30%;
max-width: 500px;
}

a {
color: #fff;
}
1 change: 1 addition & 0 deletions lib/generators/pageflow/install/install_generator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ def generate_all
invoke 'pageflow:user'
invoke 'pageflow:seeds'
invoke 'pageflow:active_admin_initializer'
invoke 'pageflow:error_pages'

rake 'pageflow:install:migrations'
end
Expand Down
15 changes: 0 additions & 15 deletions public/pageflow/404.html
View file
Open in desktop

This file was deleted.

15 changes: 0 additions & 15 deletions public/pageflow/500.html
View file
Open in desktop

This file was deleted.

40 changes: 0 additions & 40 deletions public/pageflow/error_assets/stylesheets/main.css
View file
Open in desktop

This file was deleted.

45 changes: 45 additions & 0 deletions spec/generators/pageflow/error_pages/error_pages_generator_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
require 'spec_helper'
require 'support/shared_contexts/generator'
require 'generators/pageflow/error_pages/error_pages_generator'

module Pageflow
module Generators
describe ErrorPagesGenerator, type: :generator do
it "generates 'public/pageflow/error_pages/404.html'" do
run_generator

expect(file('public/pageflow/error_pages/404.html')).to exist
end

it 'copies the fonts' do
run_generator

%w(
sourcesanspro-bold-webfont.eot
sourcesanspro-bold-webfont.svg
sourcesanspro-bold-webfont.ttf
sourcesanspro-bold-webfont.woff

sourcesanspro-regular-webfont.eot
sourcesanspro-regular-webfont.svg
sourcesanspro-regular-webfont.ttf
sourcesanspro-regular-webfont.woff
).each do |font_file|
expect(file("public/pageflow/error_pages/fonts/#{font_file}")).to exist
end
end

it 'copies the stylesheets' do
run_generator

expect(file('public/pageflow/error_pages/stylesheets/main.css')).to exist
end

it 'copies the images' do
run_generator

expect(file('public/pageflow/error_pages/images/bg.jpg')).to exist
end
end
end
end