[Feature] [WEBSITE-1239] 404 page redesign #2253

albertocorrea · 2016-12-15T05:32:09Z

JIRA

https://fameandpartners.atlassian.net/browse/WEBSITE-1239

Summary

404 new page redesign

Check error pages on staging
Fix CI state

sfate · 2016-12-15T09:16:55Z

public/404.html

@@ -5,53 +5,39 @@
  <style type="text/css">
    @import url(//fonts.googleapis.com/css?family=Forum);

-    body { background-color: #fff; color: #333; text-align: center; font-family: arial, sans-serif; font-size: 14px; }
-    a{
+    body { font-family: "brandon-grotesque",sans-serif; background-color: #fff; color: #333; text-align: center; font-family: arial, sans-serif; font-size: 20px; }


Why this even one line?
Would be much readable to have it line by line.

Agreed. No need to inline nothing here :)

tiagoamaro · 2016-12-15T10:43:49Z

Now, for the missing feature: applying the application layout for the new 404 html.

mmoraes · 2016-12-15T13:37:33Z

public/404.html

+<div class="dialog">
+  <h1>Looking for something?</h1>
+  <h2><em>Sorry,</em> this page doesn't exist.</h2>
+  <p>Try our <a href="https://www.fameandpartners.com/?cf=404">homepage</a> to see what's new and start customizing.</p>


How about just using href="/", so that it redirects to AU or US' homepages accordingly?

Would it have any side-effect? @tiagoamaro @sfate

@mmoraes, negative. Actually this is wanted. Good catch ⚾️.

@mmoraes very good point.
It shouldn't have any side-effects.

albertocorrea · 2016-12-16T05:02:19Z

@mmoraes I fix the css format and link. Also confirmed with Jess that this should be vertically centered.

albertocorrea · 2016-12-22T02:00:31Z

@sfate @mmoraes I follow https://mattbrictson.com/dynamic-rails-error-pages as per Tiago's suggestion. According to me is looking ok but I haven't done this in a long time. Let me know if we need to fix anything. Also want to make sure I that the 500 error page will still work as before.

sfate · 2016-12-29T16:53:55Z

@albertocorrea @mmoraes @tiagoamaro
I've updated code for rendering errors templates in application's layout.
Please take a look on 422 and 500 pages. I think we need to tweak design for them as well.

mmoraes · 2016-12-29T19:22:58Z

@sfate With the new error rendering templates I guess the not_found.html.slim file is not needed anymore, right?

sfate · 2016-12-30T10:40:30Z

@mmoraes I've removed it previously. Seems something went bad after your merge with master.
Please avoid merges in development branches. You can always use rebase, e.g. git rebase master.

sfate · 2016-12-30T14:41:26Z

@mmoraes FYI: I've rebased branch and removed merge commit.

mmoraes · 2016-12-30T14:42:41Z

Thank you @sfate !

mmoraes · 2017-01-03T21:25:09Z

What's left for this PR? I think we can merge it...? @sfate

sfate · 2017-01-04T09:23:03Z

@mmoraes
I think we only need to get back 500.html to public folder.
And reconfigure a bit our nginx config for fallback error pages.
We need only to keep this page for 503 status when rails server is not reachable.

I think we need to discuss this w/ @tiagoamaro first.

tiagoamaro · 2017-01-09T08:56:27Z

app/controllers/concerns/errors.rb

+    extend ActiveSupport::Concern
+
+    included do
+      rescue_from Exception,                           with: -> { render_error(code: 500) }


@sfate,

Don't think rescuing 500 errors is necessary:

500 errors should never happen. Don't think it's a need to turn them into a pretty dynamic ERB

Deleting 500 static HTML (public/500.html) will result in nginx errors if the 500 dynamic page has errors by itself

@tiagoamaro Also though about this as well.
Will move it back.

tiagoamaro · 2017-01-09T08:58:48Z

app/controllers/concerns/errors.rb

+      rescue_from ActionController::UnknownController, with: -> { render_error(code: 404) }
+      rescue_from ActionController::UnknownAction,     with: -> { render_error(code: 404) }
+      rescue_from ActionController::RoutingError,      with: -> { render_error(code: 404) }
+    end


Since we're on Rails 3, do you think it would be possible replacing those ActiveRecord/ActionController exceptions to something like this: https://mattbrictson.com/dynamic-rails-error-pages ?

@tiagoamaro
I don't like this behavior (from the article) by this points:

We need to add additional controller and change routes.

It wouldn't work for most cases. I've tried this approach w/ raising ActiveRecord::NotFound exception and it showed up an a standard rails error page (instead of 404's page from controller).

Why I'd prefer solution with catching exceptions in concern:

We can easily tweak error pages/paths/additional data for all error pages.

We have all necessary errors listed at one place.

We can change behavior of raising errors.

We do not need to touch rotes.

Agreed. Continue and will check it back when the 500 HTML is restored.

@tiagoamaro returned back 500 static page: 4930a91

@sfate already deploying your changes to staging. Nice timing.

tiagoamaro

Ready to QA.

tiagoamaro · 2017-01-11T10:28:59Z

@albertocorrea @mmoraes, any more changes to the 404 design? This was only missing the backend part, right?

mmoraes · 2017-01-11T11:30:21Z

@tiagoamaro Yes, that was the last pending part (backend). I think it's all good now.

tiagoamaro · 2017-01-11T12:21:50Z

This doesn't seem to be working on staging. Marking it as a WIP and moving back to in progress @sfate.

tiagoamaro · 2017-02-06T23:10:00Z

@sfate incredible reason to have acceptance tests: Afterpay routes are getting mounted after main app routing, which means anything from the Afterpay engine is being seen, since match '*path' matches anything :)

…tch) Ref: rails/rails#671

sfate · 2017-02-17T01:24:51Z

@tiagoamaro updated code.. routes should work now as expected 😃

tiagoamaro

LGTM

* 404 page redesign * Fix css format * Fix link destination * Remove old 404 file * Add new controller and helper * Add new 404 template * Add new route to the 404 page * Tell rails to use tempaltes * Error pages sass * Catch errors with custom error pages yielded w/ application layout * update styling and put content inside .container * Optimize calls + raise exception for development mode * Return back static 500 page * Make rails app available to catch RoutingError (raised by ActionDispatch) Ref: rails/rails#671 * Fix not found routes spec * Fix unmactched routes catch * Fix specs

albertocorrea added front-end needs-approval needs-review labels Dec 15, 2016

albertocorrea self-assigned this Dec 15, 2016

albertocorrea requested review from mmoraes and ElGrecode December 15, 2016 05:32

sfate reviewed Dec 15, 2016

View reviewed changes

mmoraes reviewed Dec 15, 2016

View reviewed changes

albertocorrea added the wip label Dec 20, 2016

albertocorrea removed the wip label Dec 22, 2016

sfate force-pushed the feature/404-page-redesign-WEBSITE-1239 branch from 07d1185 to 6bf0207 Compare December 28, 2016 15:10

sfate force-pushed the feature/404-page-redesign-WEBSITE-1239 branch from 07cf0ad to 95b5bed Compare December 30, 2016 14:40

tiagoamaro reviewed Jan 9, 2017

View reviewed changes

tiagoamaro approved these changes Jan 11, 2017

View reviewed changes

tiagoamaro added the on-qa label Jan 11, 2017

tiagoamaro added wip and removed on-qa labels Jan 11, 2017

albertorenteria and others added 16 commits February 16, 2017 17:23

404 page redesign

a753792

Fix css format

c302215

Fix link destination

5276732

Remove old 404 file

11e1ee2

Add new controller and helper

c6e4587

Add new 404 template

6488abf

Add new route to the 404 page

47fc5cf

Tell rails to use tempaltes

e1bcb68

Error pages sass

6090944

Catch errors with custom error pages yielded w/ application layout

9bb059c

Optimize calls + raise exception for development mode

0e6dbf8

update styling and put content inside .container

4d06368

Return back static 500 page

d5f7492

Make rails app available to catch RoutingError (raised by ActionDispa…

89c8e57

…tch) Ref: rails/rails#671

Fix not found routes spec

bf0a952

Fix unmactched routes catch

ab239d5

sfate force-pushed the feature/404-page-redesign-WEBSITE-1239 branch from 87ae851 to ab239d5 Compare February 17, 2017 01:23

tiagoamaro approved these changes Feb 17, 2017

View reviewed changes

Fix specs

62843b2

sfate added back-end and removed needs-approval on-qa labels Feb 17, 2017

sfate self-assigned this Feb 17, 2017

sfate merged commit eca9243 into master Feb 21, 2017

sfate deleted the feature/404-page-redesign-WEBSITE-1239 branch February 21, 2017 15:01

dagnar unassigned sfate Jun 20, 2017

reginaldrivera unassigned albertocorrea May 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] [WEBSITE-1239] 404 page redesign #2253

[Feature] [WEBSITE-1239] 404 page redesign #2253

albertocorrea commented Dec 15, 2016 •

edited by sfate

sfate Dec 15, 2016

tiagoamaro Dec 15, 2016

tiagoamaro commented Dec 15, 2016

mmoraes Dec 15, 2016

tiagoamaro Dec 15, 2016

sfate Dec 15, 2016

albertocorrea commented Dec 16, 2016

albertocorrea commented Dec 22, 2016

sfate commented Dec 29, 2016

mmoraes commented Dec 29, 2016

sfate commented Dec 30, 2016

sfate commented Dec 30, 2016

mmoraes commented Dec 30, 2016

mmoraes commented Jan 3, 2017

sfate commented Jan 4, 2017 •

edited

tiagoamaro Jan 9, 2017

sfate Jan 9, 2017

tiagoamaro Jan 9, 2017

sfate Jan 9, 2017

tiagoamaro Jan 9, 2017

sfate Jan 11, 2017

tiagoamaro Jan 11, 2017

tiagoamaro left a comment

tiagoamaro commented Jan 11, 2017

mmoraes commented Jan 11, 2017

tiagoamaro commented Jan 11, 2017

tiagoamaro commented Feb 6, 2017

sfate commented Feb 17, 2017

tiagoamaro left a comment

[Feature] [WEBSITE-1239] 404 page redesign #2253

[Feature] [WEBSITE-1239] 404 page redesign #2253

Conversation

albertocorrea commented Dec 15, 2016 • edited by sfate

JIRA

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagoamaro commented Dec 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertocorrea commented Dec 16, 2016

albertocorrea commented Dec 22, 2016

sfate commented Dec 29, 2016

mmoraes commented Dec 29, 2016

sfate commented Dec 30, 2016

sfate commented Dec 30, 2016

mmoraes commented Dec 30, 2016

mmoraes commented Jan 3, 2017

sfate commented Jan 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagoamaro left a comment

Choose a reason for hiding this comment

tiagoamaro commented Jan 11, 2017

mmoraes commented Jan 11, 2017

tiagoamaro commented Jan 11, 2017

tiagoamaro commented Feb 6, 2017

sfate commented Feb 17, 2017

tiagoamaro left a comment

Choose a reason for hiding this comment

albertocorrea commented Dec 15, 2016 •

edited by sfate

sfate commented Jan 4, 2017 •

edited