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

Made debug, exception and not_found pages responsive #1304

Closed
wants to merge 5 commits into from

Conversation

jhthorsen
Copy link
Member

@jhthorsen jhthorsen commented Dec 21, 2018

Summary

This PR makes the debug, exception and not_found pages mobile friendly. See screenshots below for a quick overview of the changes.

  • Menu links is hidden under a hamburger toggle on small screens.
  • The debug pages on mobile will allow the pre/table elements to scroll horizontally.
  • The images on not_found and exception pages are in center on mobile.
  • Should lib/Mojolicious/resources/templates/mojo/perldoc.html.ep be deleted?

Motivation

The motivation for this PR is to make Mojolicious more mobile friendly. It will also make the diff for mojolicious/mojolicious.org#6 smaller.

References

not found production

exception production

exception development

not found menu open development

@jhthorsen jhthorsen self-assigned this Dec 21, 2018
jhthorsen pushed a commit to mojolicious/mojolicious.org that referenced this pull request Dec 21, 2018
vertical-align: middle;
}
#mojobar-brand picture:after {
content: "\2261";
Copy link
Member

Choose a reason for hiding this comment

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

How portable is this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've tested it through browserstack.com on the following devices/browsers:

  • Iphone XS
  • Iphone 8
  • Google Pixel 2
  • Galaxy S6
  • Windows Phone 8.1
  • Internet Explorer 9
  • Firefox 60
  • Google Chrome 60

It does not work in IE8, but I don't consider that a problem, since the target is small screen devices and not many users are on IE8 - Especially not developers.

The appearance is also a bit different, so if we want it to be pixel perfect, then I have to replace it with an image/icon.

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the target for these internal pages? I mean, do we care about IE9 and older? I was hoping we could drop support for anything that is used less than 0.25%.

There's a nice slider on the settings page on https://caniuse.com/ that can give a quick guide of what needs to be supported.

Browser usage 0.25%

Copy link
Member

@kraih kraih left a comment

Choose a reason for hiding this comment

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

Just commenting since @jhthorsen requested a review from me. The rather arbitrary max-width choices without explanation bother me. And therefore i think i would vote against this approach. Specifically i would like to know why this approach is preferable to a Bootstrap solution.

@jhthorsen
Copy link
Member Author

What is a "Bootstrap solution"? Does that mean depending on all of the CSS from bootstrap or are you talking specifically about the breakpoints they use?

There’s two ways to set breakpoints: One way is to do it based on the device, which is the default in Bootstrap. Those values however are sort of random values, since there’s so many screen widths nowadays - especially if you take rotation of screens into consideration. The other way to set a breakpoint is to look at the content you have and see when it has to change layout. That’s what I choose here, but I also rounded it up to a value that is easier to remember.

@kraih
Copy link
Member

kraih commented Jan 3, 2019

One of our spin-off projects already uses Bootstrap very successfully to provide a responsive UI. (And another plugin in the mojolicious org does too...) I'm afraid your arguments have not convinced me that your solution is a better choice, it just doesn't feel right.

@jhthorsen
Copy link
Member Author

Ok. Thanks for looking over the changes.

@jhthorsen jhthorsen closed this Jan 3, 2019
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.

2 participants