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

Mobile friendliness #273

Closed
wants to merge 2 commits into from
Closed

Mobile friendliness #273

wants to merge 2 commits into from

Conversation

domenkozar
Copy link
Member

@domenkozar domenkozar commented Apr 29, 2019

Google Console reports:

2019-04-29-104348_1221x396_escrotum

I hope this resolves at least a few :)

cc @samueldr

@domenkozar domenkozar requested a review from grahamc April 29, 2019 03:44
@samueldr
Copy link
Member

samueldr commented Apr 29, 2019

I'm thinking this won't fix anything in there, that the 778~780 pages are the mailing list archives and the misc. pages under ~eelco (and maybe a few other stragglers like non-bootstrapped generated manuals).

I say this considering the error "Viewport not set", while it is definitely set for the main website.

Is there a list of pages that failed validation, or only that unhelpful count?

I would be surprised if the framework that once set the gold standard for mean viable responsive website (bootstrap 2) would now fail criteria.

@domenkozar
Copy link
Member Author

Forgot to mention: there is a list of pages, so this aims for fixing the manual.

Viewport error however applies there as well, so I'm a bit puzzled (although manual was indeed too long due to word wrapping)

css/nixos-site.css Outdated Show resolved Hide resolved
css/nixos-site.css Outdated Show resolved Hide resolved
@@ -2,6 +2,8 @@

body {
padding-top: 50px;
/* a bit more mobile friendly */
font-size: 16px;
Copy link
Member

Choose a reason for hiding this comment

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

Since this is intended for mobile devices, let's put it into a media query:

/* Landscape phone to portrait tablet */
@media (max-width: 767px) { ... }
 
/* Landscape phones and down */
@media (max-width: 480px) { ... }

My main issue is with how everything was made with the default font size in mind, with absolute font size units, which will only bring the unchanged font sizes up.

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 understand your cautiousness, but I've checked the rendered homepage and there don't seem to be problems.

Note that 16px is the default in Bootstrap 4, so I think this goes into the right direction making text a bit easier to read.

Copy link
Member

@samueldr samueldr May 1, 2019

Choose a reason for hiding this comment

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

It's more involved than 16px; basically it relies on the browser's settings, and most defaults will end up being equivalent to 16px.

While I do agree generally with the "default 16px" being better, my previous point still stands that everything has been conceived with bootstrap's default font-size in mind, not only our additions, but bootstraps' defaults.

This includes some subtle, but imo breaking changes like the line-height set in absolute units to 20px, and some misc paddings and margins. The line-height, especially, makes the website feel much more cramped, and lines squeezed together so closely might hamper readability more.

I'm not saying "block this absolutely on the font size", but I would strongly lean towards cautiousness and not update it without a full review of the implementation of the design, even if not redesigning the appearance of the website. Things have evolved everywhere since bootstrap 2's inception.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could.you provide an example of such occourance?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know of specifics for padding and margins, almost nothing is set side-by-side in the site, so that part is not as much an issue.

Though, most of the text in the site inherits the absolute unit line-height.

image
image

image
image

The reduced line spacing hampers readability, it should at least be increased accordingly. To be equivalent, it should be set to 23px on body.

Here's how it would look with a line-height of 23px on the body.

image

@domenkozar
Copy link
Member Author

Sadly lost interested in this, maybe someone else will pick it up

@domenkozar domenkozar closed this Jun 11, 2019
@garbas garbas deleted the mobile-improvements branch August 26, 2020 19:27
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.

None yet

2 participants