Welcome! Log In Create A New Profile

Advanced

Re: [PATCH 3 of 3] Stylesheet style changes.

Maxim Dounin
August 02, 2023 01:38PM
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@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
Subject Author Views Posted

[PATCH 0 of 3] nginx.org usability enhancements

Liam Crilly via nginx-devel 372 July 24, 2023 10:22AM

[PATCH 1 of 3] CSS as file.

Liam Crilly via nginx-devel 118 July 24, 2023 10:24AM

[PATCH 2 of 3] Responsive menu.

Liam Crilly via nginx-devel 124 July 24, 2023 10:26AM

[PATCH 3 of 3] Stylesheet style changes.

Liam Crilly via nginx-devel 115 July 24, 2023 10:28AM

Re: [PATCH 3 of 3] Stylesheet style changes.

Maxim Dounin 110 August 02, 2023 01:38PM

Re: [PATCH 2 of 3] Responsive menu.

Maxim Dounin 108 August 02, 2023 01:38PM

Re: [PATCH 1 of 3] CSS as file.

Maxim Dounin 103 August 02, 2023 01:38PM

Re: [PATCH 1 of 3] CSS as file.

Liam Crilly via nginx-devel 153 August 04, 2023 11:50AM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 91
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 500 on July 15, 2024
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready