Hello!
On Mon, Jul 24, 2023 at 02:25:16PM +0000, Liam Crilly via nginx-devel wrote:
> # HG changeset patch
> # User Liam Crilly <liam.cri...@nginx.com>
> # Date 1690207285 -3600
> # Mon Jul 24 15:01:25 2023 +0100
> # Node ID c4018ab31dc52632f470298fcc7cece1fd8f57b9
> # Parent df1cf98cf8f50eb1770d966aed583d21e481558b
> Responsive menu.
>
> Nav menu is now hidden on narrow displays, collapsing to a "hamburger"
> menu icon in the banner. Menu items are now an unordered list instead of
> simple text markup.
In no particular order:
- It looks like this changeset contains more than one logical
change. It is generally a good idea to keep distinct changes in
separate changesets as long as it is possible. A useful rule of
thumb is to consider using more than one changeset as long as
you have to mention more than one item in the commit log.
- It looks like there are multiple style issues in the change: it
fails to follow the style you've just introduced in the previous
changeset, and uses style definitions without spaces and empty
lines.
- Design-wise, banner is expected to be on top of the site, above
anything. That's basically reflects the fact that it was, in
fact, added to the site as external element. With the suggested
change, this is no longer true, and hamburger interferes with
banner. This needs a better solution. Removing the banner
might be an option, it clearly meaningless now.
- Resulting menu seems to be non-scrollable on small screens, and
detaches from the hamburger when the page is scrolled.
- This change demonstrates very well that using multiple
language-specific styles makes development hard: we are already
seeing bit rot for he and cn languages, and duplicate work for
en/ru.
See below for more comments.
>
> diff -r df1cf98cf8f5 -r c4018ab31dc5 css/style_en.css
> --- a/css/style_en.css Mon Jul 24 14:59:57 2023 +0100
> +++ b/css/style_en.css Mon Jul 24 15:01:25 2023 +0100
> @@ -29,10 +29,22 @@
>
> #menu {
> float: right;
> - width: 11em;
> - padding: 0 .5em 1em .5em;
> + width: 13em;
> + padding: 0;
> border-left: 2px solid #DDD;
> }
> +.nav ul{
> + margin:0;
> + padding:20px;
> +}
> +.nav h1{
> + padding:0 20px;
> +}
> +.nav ul li{
> + list-style-type:none;
> + padding:0;
> + margin:0;
> +}
>
> #content {
> margin-right: 13.5em;
> @@ -43,9 +55,7 @@
> display: block;
> font-size: 3em;
> text-align: left;
> - height: .7em;
> - margin: 0;
> - margin-bottom: .5em;
> + margin: 0.5rem 0 0 0;
> }
>
> h1 img {
> @@ -205,3 +215,125 @@
> width:100%;
> height:100%;
> }
> +
> +@media screen and (max-width:768px) {
Overall, I would expect minimal media-specific changes for already
defined elements, such as display changes.
> + #main {
> + padding: 20px;
> + min-width: inherit;
> + }
> + #main #content {
> + width: 100%!important;
> + padding: 0;
> + border: none;
> + }
> +
> + #banner-content {
> + max-width: 70vw;
> + }
> +
> + #menu {
> + text-align: left;
> + }
> +
> + /* Menu Mobile */
It might be a good idea to avoid useless comments.
> + :root {
> + --white: #f9f9f9;
> + --black: #000;
> + --gray: #85888C;
> + --green: #b6d7a8;
> + color-scheme: light dark;
> + } /* variables*/
> +
> + /* Nav menu */
> + .nav {
> + width: 15rem;
> + height: 100%;
> + max-height: 0;
> + position: fixed;
This uses "position: fixed;", and therefore menu cannot be
scrolled when it does not fit on screen.
Also, hamburger uses "position: absolute;", and together with
fixed menu this results in hamburger and menu being when the page
itself is scrolled.
> + top: 50px;
> + right: 0;
> + border-left: 1px solid #909090;
> + background-color: var(--white);
Using variable colors along with non-variable looks wrong.
> + overflow: hidden;
> + transition: .5s ease-in-out;
> + }
> + .nav ul {
> + margin: 0;
> + padding: 20px;
> + }
> + .nav h1 {
> + padding: 0 20px;
> + }
> + .nav ul li {
> + list-style-type: none;
> + padding:0;
> + margin:0;
> + }
> +
> + .hamb {
> + cursor: pointer;
> + float: right;
> + position: absolute;
> + top: 0;
> + right: 20px;
> + height: 30px;
> + width: 70px;
> + padding-left: 20px;
> + padding-top: 20px;
> + z-index: 1100;
> + }
> + .hamb-line {
> + background: var(--green);
> + display: block;
> + height: 5px;
> + position: relative;
> + width: 44px;
> + border-radius:3px;
> +
> + }
> + .hamb-line::before,.hamb-line::after {
> + background: var(--green);
> + content: '';
> + display: block;
> + height: 100%;
> + position: absolute;
> + transition: all .2s ease-in-out;
> + width: 100%;
> + border-radius:3px;
> + }
> + .hamb-line::before{
> + top: 10px;
> + }
> + .hamb-line::after{
> + top: -10px;
> + }
> + .side-menu {
> + display: none;
> + } /* Hide checkbox */
> + .side-menu:checked ~ .nav{
> + max-height: 100%;
> + top: 50px;
> +
> + }
> + .side-menu:checked ~ .hamb .hamb-line {
> + background: transparent;
> + }
> + .side-menu:checked ~ .hamb .hamb-line::before {
> + transform: rotate(-45deg);
> + top: 0;
> + }
> + .side-menu:checked ~ .hamb .hamb-line::after {
> + transform: rotate(45deg);
> + top: 0;
> + }
> +
> + code {
> + white-space: pre-line;
> + }
> +}
> +
> +@media screen and (min-width:768px){
> + .side-menu,.hamb-line {
> + display: none;
> + }
> +}
> diff -r df1cf98cf8f5 -r c4018ab31dc5 css/style_ru.css
> --- a/css/style_ru.css Mon Jul 24 14:59:57 2023 +0100
> +++ b/css/style_ru.css Mon Jul 24 15:01:25 2023 +0100
Skipped style_ru.css, as it is exactly identical to style_en.css.
[...]
> diff -r df1cf98cf8f5 -r c4018ab31dc5 xsls/banner.xsls
> --- a/xsls/banner.xsls Mon Jul 24 14:59:57 2023 +0100
> +++ b/xsls/banner.xsls Mon Jul 24 15:01:25 2023 +0100
> @@ -21,7 +21,7 @@
> fetch("!banner_link ()")
> .then((response) => response.text())
> .then((resp) => \{
> - document.getElementById("banner").innerHTML = resp;
> + document.getElementById("banner-content").innerHTML =
> resp;
> \})
> .catch((error) => \{
> console.warn(error);
> diff -r df1cf98cf8f5 -r c4018ab31dc5 xsls/body.xsls
> --- a/xsls/body.xsls Mon Jul 24 14:59:57 2023 +0100
> +++ b/xsls/body.xsls Mon Jul 24 15:01:25 2023 +0100
> @@ -18,20 +18,27 @@
> <body>
>
> <div id="banner">
> + <div id="banner-content"></div>
> </div>
>
> <div id="main">
> - <div id="menu">
> + <div id="menu" >
Unrelated whitespace change.
> + <input class="side-menu" type="checkbox" id="side-menu"/>
> + <label class="hamb" for="side-menu"><span
> class="hamb-line"></span></label>
> + <nav class="nav">
> <h1>
> X:if "@lang = 'he'" { X:attribute "align" { X:text{left} } }
> <a href="/">
> - <img src="/nginx.png" alt="nginx" />
> + <img src="/nginx.png" alt="nginx"/>
And here, and in other places.
> </a>
> </h1>
> <div>
> - !! "document(concat($XML, '/menu.xml'))
> + <ul class="mobilemenu">
Using "mobilemenu" name here is clearly wrong, as it's not just
mobile.
> + !! "document(concat($XML, '/menu.xml'))
> /menus/menu[@lang = $lang]/item";
> + </ul>
> </div>
> + </nav>
> </div>
>
> <div id="content">
> diff -r df1cf98cf8f5 -r c4018ab31dc5 xsls/menu.xsls
> --- a/xsls/menu.xsls Mon Jul 24 14:59:57 2023 +0100
> +++ b/xsls/menu.xsls Mon Jul 24 15:01:25 2023 +0100
> @@ -13,6 +13,8 @@
> -- "menu/item[@href = $LINK]", etc.
> -->
>
> + <li>
> +
> X:if "@href = $LINK" {
> X:if "$YEAR and @href='/'" {
> <a href="./"> news </a> <br/>
> @@ -21,7 +23,6 @@
> }
>
> } else {
> -
> <!--
> -- If a menu item has the switchlang attribute, then it will point
> -- to the same document in the specified language.
> @@ -66,33 +67,34 @@
>
> X:if "@lang" { X:text { [} !{@lang} X:text {]}}
> }
> -
> - <br/>
> }
> + </li>
> }
>
>
> X:template = "menu/item[@year]" {
> X:if "$YEAR or $LINK='/'" {
> + <li>
> X:if "$YEAR=@year" {
> - !{@year} <br/>
> + !{@year}
> } else {
> X:if "@href" { <a href="{@href}"> !{@year} </a> }
> - <br/>
With this change, an empty line separating years from the rest of
the menu is lost. As of now, the empty line is provided for:
<item year="" />
item in menu.xml.
> }
> + </li>
> }
> }
>
>
> X:template = "menu/item[starts-with(@href, 'http://') or starts-with(@href,
> 'https://')]" {
> + <li>
> <a href="{@href}"> !{ normalize-space(text()) } </a>
> X:if "@lang" { X:text { [} !{@lang} X:text {]}}
> - <br/>
> + </li>
> }
>
>
> X:template = "menu/item[not(@href) and not(@year)]" {
> - !{ normalize-space(text()) } <br/>
> + <li>!{ normalize-space(text()) } <br/></li>
> }
>
> }
> diff -r df1cf98cf8f5 -r c4018ab31dc5 xsls/style.xsls
> --- a/xsls/style.xsls Mon Jul 24 14:59:57 2023 +0100
> +++ b/xsls/style.xsls Mon Jul 24 15:01:25 2023 +0100
> @@ -7,6 +7,7 @@
>
> X:template style (lang) {
>
> + <meta name="viewport"
> content="width=device-width,initial-scale=1"></meta>
> <link rel="stylesheet" href="/css/style_{$lang}.css"></link>
>
> }
Overall, this clearly needs more work, and probably needs to be
split into smaller changes.
--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
