Conversation
dscho
left a comment
dscho
left a comment
There was a problem hiding this comment.
This looks great!
Just a few things that I'd like to see fixed before merging.
| <small>... one iteration at a time... </small> | ||
| {{- end -}} | ||
| <div class="banner"> | ||
| {{- if eq .Kind "home" }} |
There was a problem hiding this comment.
I admit, this was no regression, it was intentional.
As a user navigating from the homepage to the other non-homepages on desktop, I got the impression that the banners weren't rendering properly, they didn't look right compared to the homepage banner.
I was honestly unsure if the table banner layout itself had a bug causing it to look different than the homepage's on both desktop and mobile. Given how infrequently this repo is updated, instead of potentially waiting a while for an answer before pushing, I just lead with the assumption I was fixing a bug and that someone would tell me otherwise if not!
So here's my case presented to you for changing it and why it lands in scope to affect desktop.
- Inconsistent branding without an obvious justification.
Generally, it's desirable to have the logo and other brand elements appear consistently across similar contexts to enhance recognition. The pages all share a similar body structure and background and they position the banner in the same area, so the banners are basically serving the same purpose. The visual concept behind the two banner types is basically the same:
two brand blocks parallel with each other
...but with significantly different sizing and spacing proportions in their implementations.
Because the concept is the same but the implementation is different enough within the same context, it reads as inconsistent rather than intentional. It doesn't really look like the branding banner is being experienced in a new way, it looks like either a bug is rendering it differently than the homepage or a decision couldn't be made on how the banner looked best so both variants were included.
- Tables with image content don't behave well on mobile.
Cells don't stack or reflow, and it's harder to control how images scale within them without fighting the table structure. The banner table already looked inconsistent with the homepage's banner on desktop, so rather than fighting to make it work responsively, it made more sense to just remove it, solving both the branding inconsistency affecting desktop and the mobile issues in one move.
I updated my commit message to make clear that this is a deliberate unification of page banner layouts.
Though I recommend against it, if you would prefer me to restore the table structure layout for the non-homepages, I can do that and try to figure out a way to get it to behave properly on mobile. Let me know if that's what you would prefer instead.
There was a problem hiding this comment.
Thank you for taking the time to explain the rationale. Explained like I am 5, this actually makes total sense to me. Thank you for indulging me!
| pre { | ||
| background-color: rgb(247, 243, 239); | ||
| border: 1px solid #c5bbb0; | ||
| border-radius: 0.5em; | ||
| padding: 1em; | ||
| overflow-x: auto; | ||
| } | ||
| code { | ||
| background-color: rgb(247, 243, 239); | ||
| padding: 0.1em 0.3em; | ||
| border-radius: 0.25em; | ||
| } | ||
| pre code { | ||
| background-color: transparent; | ||
| padding: 0; | ||
| } |
There was a problem hiding this comment.
I would really like to learn from this. To that end, would you mind extending the commit message quite a bit? I'd like it more in line with https://github.blog/2022-06-30-write-better-commits-build-better-projects/, in particular with a strong focus on this part:
| What you’re doing | Why you’re doing it | |
|---|---|---|
| High-level (strategic) | Intent (what does this accomplish?) | Context (why does the code do what it does now?) |
| Low-level (tactical) | Implementation (what did you do to accomplish your goal?) | Justification (why is this change being made?) |
There was a problem hiding this comment.
Sure, I appreciate your enthusiasm for this update, I'll provide a more comprehensive and clear set of commit msgs.
| background-image: linear-gradient(#d7cdc0ff, #fdf7f6ff); | ||
| margin-right: 10%; | ||
| margin-left: 10%; | ||
| overflow-wrap: break-word; |
There was a problem hiding this comment.
Is this a fixup for dad29bc that would want to be squashed into that commit instead of living on its own? Also, the commit message does not help me understand the problem, the context, and why the proposed solution is the best way to solve the problem. I would love it to help me, though.
There was a problem hiding this comment.
Those are scoped separately, I will update the commit msg to make that clear.
On mobile, without an explicit viewport, pages can render at desktop width, forcing a two-handed pinch/zoom/pan "panzoom dance" just to read content, instead of simply scrolling vertically. Add the standard viewport meta tag in the base template so initial scale matches device width. This is a necessary foundation, not a complete fix: other overflow sources (long unbroken content, wide elements) can still cause horizontal scrolling and are handled in follow-up commits.
Banner images were not scaling on mobile, so they appeared oversized/disproportionate and triggered horizontal page overflow. Apply responsive image sizing in `.banner` (`max-width: 100%`, `height: auto`) so banner images scale correctly. Non-home pages also used a table-based banner layout, which is generally not mobile-friendly and makes it harder to position, contain, and scale content within smaller viewports. It also introduced unnecessary branding inconsistency across desktop and mobile: the home page and non-home pages presented essentially the same banner content with different proportions and visual behavior but still serving the same purpose and content. Rather than making the table layout itself responsive, remove it entirely. The home page already had a working non-table structure, and a single shared layout is simpler to maintain and style. Remove that page-kind split and use one shared banner structure (logo + subtitle in .banner) so banner behavior and branding are consistent on mobile and desktop across the site.
Long code lines can overflow narrow screens and trigger horizontal page scrolling. For code, wrapping/breaking tokens is undesirable because it hurts copy/paste and readability fidelity. Style `pre` as an explicit container (background, border, radius, padding) and enable `overflow-x: auto` so overflow is contained inside the code block instead of the page. The container styling, especially the border, is intentional and important: it makes the block read as a distinct container, which makes it easier to recognize when it is scrollable. Style inline `code` to match the visual language, and reset `pre code` background and padding so nested code does not get double styling. Result: on mobile, overflowing code stays unbroken and scrolls within a clearly visible bounded container; on desktop, behavior remains normal when no overflow occurs.
Long unbroken text (currently seen with URLs in the architecture page) can force horizontal page overflow on mobile. Apply `overflow-wrap: break-word` at the `body` level as a global safeguard and default policy: regular content should wrap rather than widen the viewport. This is intentionally global instead of targeting specific elements, so new non-specialized content also gets safe wrapping by default. Specialized components (for example code blocks and tables) can keep their own overflow behavior where preserving layout/fidelity is more important.
Desktop margins are acceptable, but on small screens they consume too much of the viewport and leave content unnecessarily constrained. At <=767px, reduce body side margins from 10% to 3% so more width is available for ordinary content. This lowers avoidable wrapping and reduces the chance of horizontal overflow triggered by overly narrow content area.
On smaller screens, the desktop-sized banner title does not fit on one line and can break within the title word at arbitrary character boundaries. At <=450px, reduce `big` from 400% to 300% so the title stays on one line and remains readable.
Graphviz diagrams are rendered/injected as plain `<img>` elements outside the banner container, and on mobile they were causing horizontal page overflow. The previous responsive image rule was scoped to `.banner img`, so those non-banner images could exceed viewport width and trigger horizontal scrolling. Move `max-width: 100%` and `height: auto` from `.banner img` to a global `img` rule. This sets a site-wide default policy that images should fit their container, while still preserving aspect ratio. This immediately fixes horizontal overflow for Graphviz images and also provides the same protection for future non-banner images by default.
ritorhymes
force-pushed
the
mobile-friendly
branch
from
38776e1 to
85e377b
Compare
dscho
left a comment
dscho
left a comment
There was a problem hiding this comment.
Thank you for improving the commit messages so much. The love and care you put into them clearly shows and I appreciate it a lot. The comment messages now tell a really compelling story, are pleasant to read, and I am super happy to merge it. Thank you so much!
2 participants
Summary
This PR closes #19 and makes the site (all 3 pages) mobile responsive without changing the intended UI on desktop, except for the non-homepage table banner which has been removed to remain stylistically consistent with the homepage banner, responsive and more maintainable. No content has been edited.
A live demo can be tested here https://gitgitgadget.ritovision.com
What has changed
Before and After Screenshots
Homepage - BEFORE
Homepage - AFTER
Architecture - BEFORE
Architecture - AFTER
Reply-to-this - BEFORE
Reply-to-this - AFTER
Desktop Homepage - BEFORE
Desktop Homepage - AFTER
Notice the layout still looks and scales the same