[PATCH 3 of 3] Stylesheet style changes.

[Maxim Dounin]

Hello!

On Mon, Jul 24, 2023 at 02:26:41PM +0000, Liam Crilly via nginx-devel wrote:

> # HG changeset patch
> # User Liam Crilly <liam.crilly at nginx.com>
> # Date 1690207410 -3600
> #      Mon Jul 24 15:03:30 2023 +0100
> # Node ID 29929155db5ad60e9f9c201d74fb73889287848f
> # Parent  c4018ab31dc52632f470298fcc7cece1fd8f57b9
> Stylesheet style changes.
> 
> Optimized for readability, including support for dark mode.

Overall, I tend to think these changes indeed improve readability.

But certainly there are objections to some of the changes, such as 
grey background and border for configuration examples (which was 
specifically avoided during previous changes to styles), or grey 
background and padding for "<code>" in-line elements (which simply 
looks ugly, especially when the element is in quotes).

Overall, this needs more work, notably needs to be split into 
multiple patches with one patch per logical change to make reviews 
actually possible.

> 
> diff -r c4018ab31dc5 -r 29929155db5a css/style_en.css
> --- a/css/style_en.css	Mon Jul 24 15:01:25 2023 +0100
> +++ b/css/style_en.css	Mon Jul 24 15:03:30 2023 +0100
> @@ -9,13 +9,12 @@
>  }
>  
>  #banner {
> -    background: black;
> -    color: #F2F2F2;
> +    background: #000;
> +    color: #f2f2f2;
>      line-height: 1.2em;
>      padding: .3em 0;
> -    box-shadow: 0 5px 10px black;
> +    box-shadow: 0 5px 10px #000;

These are clearly style changes.  If at all, please keep style 
changes separate from functional changes.

>  }
> -
>  #banner a {
>      color: #00B140;
>  }

And this is an unrelated whitespace corruption.

> @@ -24,7 +23,7 @@
>      text-align: left;
>      margin: 0 auto;
>      min-width: 32em;
> -    max-width: 64em;
> +    max-width: 66em;
>  }
>  
>  #menu {
> @@ -46,9 +45,9 @@
>      margin:0;
>  }
>  
> - #content {
> +#content {

This change looks like a result of whitespace damage in the first 
patch of the series, and needs to be fixed there (if at all).

>      margin-right: 13.5em;
> -    padding: 0 .2em 0 1.5em;
> +    padding: 0 2.5em 0 1.5em;
>  }
>  
>  h1 {
> @@ -63,11 +62,23 @@
>  }
>  
>  h2 {
> -    text-align: center;
> +    font-size: 2rem;
> +    line-height: 3.25rem;
> +    margin: 1rem 0 .5rem 0;
> +}
> +
> +h4 {
> +    font-size: 1.5rem;
> +    margin: 2rem 0 .5rem 0;
> +}
> +/* Center field override */
> +center h4 {
> +    text-align: left!important;
>  }

As mentioned in the review of the previous patch, please avoid 
useless comments.

Also, this particular style rule probably shouldn't be here at 
all: instead, consider removing "<center>" tag from the markup in 
relevant xsls files.

[...]

-- 
Maxim Dounin
http://mdounin.ru/