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

Styling for error pages #233

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@anonydog
Copy link

anonydog commented Jul 20, 2017

I'm trying to fix #46

The pull request is currently just a first draft. I intend to build something
together with you people.

Please take a look at my modified 404 page.

I've started from the HTML for the "Contributing" page and changed it to make a 404 message. Is this a step in the right direction? I mean... In the visual style sense?


This pull request was sent anonymously through anonydog.

There is a person behind the curtain. But the bot hides the real author.

Anonydog wants to allow people to watch how Github project maintainers behave when they don't know who is behind the keyboard.

But the code is still in its early testing stages. I want to ask you to please spend some minutes trying to identify who is the real author of this pull request. In case you manage to do it, please open an issue with the deanonymization label in the anonydog/anonydog project telling me how did you do it.

@noahgibbs

This comment has been minimized.

Copy link

noahgibbs commented Jul 20, 2017

Looks like a very reasonable approach. Off the top of my head, not sure we need to explicitly pull in all that JavaScript (example: the HighChart stuff) for a 404 page that clearly won't be doing much. You'd like a 404 to be as bulletproof and simple as possible, so that if something is seriously misconfigured it's less likely to also break.

@anonydog

This comment has been minimized.

Copy link
Author

anonydog commented Jul 20, 2017

Looks like a very reasonable approach.

Nice to hear that. My main goal was validating the visual style.

I will proceed by cleaning up the HTML code then.

@anonydog

This comment has been minimized.

Copy link
Author

anonydog commented Jul 20, 2017

This last commit removes all script tags. I didn't see any difference in the resulting page. Please check.

@noahgibbs

This comment has been minimized.

Copy link

noahgibbs commented Jul 20, 2017

Looks reasonable to me. @tgxworld?

@anonydog

This comment has been minimized.

Copy link
Author

anonydog commented Jul 21, 2017

I am a little unsure about this stylesheet reference, though:

<link
    rel="stylesheet"
    media="all"
    href="/assets/application.self-745519abfad0a8dbde6e0cd8fe1492f9d36e30a9f06a1a1785b8c57805501903.css?body=1"
    data-turbolinks-track="reload"
/>

I guess that the hex identifier gets created by some kind of asset pipeline and may change in the future. Is it safe to refer to it statically like that? Or will it break when someone changes the underlying scss stylesheet?

@noahgibbs

This comment has been minimized.

Copy link

noahgibbs commented Jul 21, 2017

It'll break when somebody changes the stylesheet. Rails automatically does those for cache-handling.

margin: 4em auto 0;
}
<link rel="stylesheet" media="screen" href="https://fonts.googleapis.com/css?family=Montserrat:400,700" data-turbolinks-track="reload" />
<link rel="stylesheet" media="all" href="/assets/application.self-745519abfad0a8dbde6e0cd8fe1492f9d36e30a9f06a1a1785b8c57805501903.css?body=1" data-turbolinks-track="reload" />

This comment has been minimized.

Copy link
@tgxworld

tgxworld Jul 21, 2017

Member

Hmm this is not going to work. Every time we recompile assets, this will break.

This comment has been minimized.

Copy link
@tgxworld

tgxworld Jul 21, 2017

Member

Also I don't think Turbolinks is available on this page.

This comment has been minimized.

Copy link
@anonydog

anonydog Jul 21, 2017

Author

Good point. I'll remove the turbolinks references.

This comment has been minimized.

Copy link
@anonydog

anonydog Jul 21, 2017

Author

What about the compiled stylesheet reference? Do you think we can get that working on a static page? Can you point me to some docs that will help?

padding: 7px 12% 0;
box-shadow: 0 3px 8px rgba(50, 50, 50, 0.17);
}
</head>

This comment has been minimized.

Copy link
@tgxworld

tgxworld Jul 21, 2017

Member

Style nitpick here, I'll prefer if we can indent the HTML tags so that it is easier to determine how the tags are nested.

This comment has been minimized.

Copy link
@anonydog

anonydog Jul 21, 2017

Author

Will do that.

@anonydog

This comment has been minimized.

Copy link
Author

anonydog commented Jul 21, 2017

Here's what I could come up with for fixing the stylesheet reference. I need
feedback about some points, though:

  1. I've added a new gem to Gemfile. Is that ok?

  2. This uses ERB. I see that the rest of the code uses HAML.

  3. I'm currently running rails assets:precompile before starting the webserver. I don't know if that breaks
    the deployment pipeline.

@noahgibbs

This comment has been minimized.

Copy link

noahgibbs commented Jul 24, 2017

Definitely better to use HAML if you need dynamic.

Adding a gem to the Gemfile is normally fine. I looked at the article that the error_page_assets gem was based on - I'm a little surprised it only supports erb (https://www.icelab.com.au/notes/precompiled-rails-static-404-and-500-pages/). Dunno. It does at least look like it's not serving the error pages dynamically, which seems like a bad idea.

@anonydog anonydog referenced this pull request Jul 24, 2017

Open

Styling for error pages #1

@anonydog

This comment has been minimized.

Copy link
Author

anonydog commented Jul 24, 2017

I've confirmed it is static. But I currently need to run rails assets:precompile before starting the webserver or else the 404 page will 404 (!). Do we need to dig deeper into that? Or is asset compilation already included in the deployment pipeline somehow?

@noahgibbs

This comment has been minimized.

Copy link

noahgibbs commented Jul 24, 2017

I believe we already build assets when deploying to production. @bmarkons, can you verify that for me?

@bmarkons

This comment has been minimized.

@anonydog

This comment has been minimized.

Copy link
Author

anonydog commented Jul 24, 2017

Nice to know. I guess the only pending issue is HAML then.

I will spare some time on that and then convert the remaining error pages.

@anonydog

This comment has been minimized.

Copy link
Author

anonydog commented Jul 26, 2017

Here are the styled pages for errors 422 and 500.

I couldn't get HAML to work with the asset pipeline. The templates will have to be ERB for the time being.

Please let me know if you have any other suggestions and/or need the commits to be squashed.

@bmarkons

This comment has been minimized.

Copy link
Collaborator

bmarkons commented Jul 26, 2017

I think we need to add these to config/initializers/assets.rb for precompilation.

@anonydog

This comment has been minimized.

Copy link
Author

anonydog commented Jul 26, 2017

My Rails is a little rusty, so I'll need a little help here.

I've tried adding this to config/initializers/assets.rb:

Rails.application.config.assets.precompile += %w( 404.html 422.html 500.html )

I couldn't see any difference. I still need to run assets:precompile before starting the webserver. And this explicit precompilation step already worked before the change. I'm a little lost on what is the expected behavior.

@noahgibbs

This comment has been minimized.

Copy link

noahgibbs commented Jul 26, 2017

If you precompile the assets in the initializer, that means that starting the server should be enough to make those assets show up. Unfortunately, just that line won't do it -- adding those to the list of assets to compile only helps if you're actually compiling assets, which doesn't happen by default when you start the server. I'm not sure how you run a Rake task on app startup outside of Rake, nor how you would run the asset precompile without running the Rake task. But presumably you'd need to do one of those two.

@anonydog anonydog force-pushed the anonydog:pullrequest-4e7b60889 branch from 480f4b2 to ea9c288 Aug 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.