-
Notifications
You must be signed in to change notification settings - Fork 576
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
Conversation
vertical-align: middle; | ||
} | ||
#mojobar-brand picture:after { | ||
content: "\2261"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How portable is this?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
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. |
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. |
Ok. Thanks for looking over the changes. |
Summary
This PR makes the debug, exception and not_found pages mobile friendly. See screenshots below for a quick overview of the changes.
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