Hello!
On Mon, Jul 24, 2023 at 02:26:41PM +0000, Liam Crilly via nginx-devel wrote:
> # HG changeset patch
> # User Liam Crilly <liam.cri...@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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
