search

ruby-bench / ruby-bench-web

Long Running Ruby Benchmarks
https://rubybench.org
MIT License
82 stars 47 forks source link

Styling for error pages #233

Closed anonydog closed 5 years ago

anonydog commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

noahgibbs commented 6 years ago

Looks reasonable to me. @tgxworld?

anonydog commented 6 years ago

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 commented 6 years ago

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

anonydog commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

bmarkons commented 6 years ago

Yes, here : https://github.com/ruby-bench/ruby-bench-web/blob/master/deploy/containers/app.yml#L59

anonydog commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

anonydog commented 6 years ago

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 commented 6 years ago

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.