Fix: Homepage illustration & CTA responsiveness + footer year update (#251) #252
Conversation
WalkthroughConfiguration updates include replacing the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes
Possibly related issues
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
assets/scss/templates/_main.scss(1 hunks)config.toml(3 hunks)layouts/index.html(1 hunks)
🔇 Additional comments (10)
assets/scss/templates/_main.scss (3)
48-76: Pagination styles are well-structured with proper interactive states.The pagination component handles spacing, sizing, and hover/active state styling consistently, with appropriate use of color variables.
7-35: Banner responsive logic aligns with PR objectives.The
.bannersection correctly toggles illustrations between desktop and mobile viewports viadisplay: noneon desktop breakpoints and conditional.illustration-mobilerendering. Padding tiers atdesktop-lg(150px/100px) anddesktop(100px/0) address the 992px–1200px viewport squeeze zone mentioned in the PR.
241-249: CTA image squeeze-zone safety cap targets the correct breakpoint.The media query
(min-width: 992px) and (max-width: 1199.98px)directly addresses the stated responsive issue for CTA section overlapping. Amax-width: 180pxconstraint prevents crowding in the 992–1200px range.Confirm that
max-width: 180pxis visually adequate to prevent the text-and-logo overlap mentioned in the PR. If the visual result still shows crowding, consider reducing further (e.g., 150px or 160px).config.toml (3)
7-7: Configuration syntax is correct.Both
pagerSize = 5andsummaryLength = 10are properly expressed as numeric (not string) values in TOML, which is appropriate for Hugo configuration.Also applies to: 9-9
67-67: Copyright year updated to 2025.
85-86: Verifycss/local.cssfile exists and is served correctly.A new CSS plugin entry points to
css/local.css, which should contain the styles defined inassets/scss/templates/_main.scss(or be the compiled output from it).Confirm that the file
static/css/local.cssexists or is generated as part of the build process. If the CSS is compiled from SCSS, ensure the build pipeline includes the SCSS compilation step forassets/scss/templates/_main.scss→static/css/local.css.layouts/index.html (4)
4-16: Banner layout correctly implements responsive image toggling.The banner uses a desktop illustration positioned absolutely with a separate mobile illustration wrapped in
.illustration-mobile. This allows the styles in_main.scssto conditionally show/hide via breakpoints, supporting the stated responsive fix for desktop viewports.
25-28: Blog post rendering with pagination is correctly structured.Posts are rendered via the "article" partial within the paginated result set. The
.Render "article"hook allows reuse of post template logic.
54-90: Pagination logic is comprehensive but uses an unusual active-page detection pattern.The Scratch-based logic for determining which page numbers to display is well-thought-out for handling lower-limit, middle, and upper-limit page ranges. However, line 84 uses
{{ if eq . $paginator }}to detect the active page, which relies on object equality comparison. This may work in Hugo but is unconventional.Verify that the active page highlighting on line 84 functions correctly by testing pagination across multiple pages locally. If the active state does not highlight properly, replace with the more explicit comparison:
-<li class="page-item{{ if eq . $paginator }} page-item active {{ end }}"> +<li class="page-item{{ if eq .PageNumber $paginator.PageNumber }} active{{ end }}">
111-134: CTA section properly respects config-driven rendering and responsive layout.The CTA block:
- Conditionally renders based on
.Site.Params.cta.enable- Uses responsive columns (
col-md-4image,col-md-8text)- Applies
mr-4margin to image for spacing alignment- Includes optional button rendering
- All content is filtered with
markdownifyandrelURL/absURLas appropriateThe
cta-imageclass integrates with the responsive safety cap defined in_main.scss(max-width: 180px at 992–1200px), addressing the stated overlapping issue.
tachyons
left a comment
tachyons
left a comment
There was a problem hiding this comment.
We don't want theme layout and style override from here as it will make theme upgrade process unnecessarily complex. So make any generic style fixes upstream first.
naman79820
force-pushed
the
fix/blog-responsive-issues-#251
branch
from
f3dd96f to
c8073f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
assets/scss/_mixins.scss (2)
152-165: Consolidate duplicate media query blocks and use existing mixin.The same media query
(min-width: 992px) and (max-width: 1199.98px)is defined twice (lines 154 and 161). Consolidate these into a single block. Additionally, a@mixin desktop-lgalready exists (line 21–24) for themax-width: 1200pxbreakpoint; consider using it or creating a reusable mixin for this specific range to improve maintainability.-/* --- 992–1200px squeeze zone fixes --- */ -@media (min-width: 992px) and (max-width: 1199.98px) { - .banner .illustration { - max-width: 540px !important; /* ensure image size sticks */ - right: 15px !important; /* prevent overlap */ - } -} -/* In the squeeze band, cap the image and give the text a nudge */ -@media (min-width: 992px) and (max-width: 1199.98px) { - .section.pt-0 .row.shadow.bg-white.p-5 > div:first-child img { - width: 180px !important; /* 320–360 works well */ - } -} +/* --- 992–1200px squeeze zone fixes --- */ +@media (min-width: 992px) and (max-width: 1199.98px) { + .banner .illustration { + max-width: 540px !important; /* ensure image size sticks */ + right: 15px !important; /* prevent overlap */ + } + + .section.pt-0 .row.shadow.bg-white.p-5 > div:first-child img { + width: 180px !important; + } +}
154-165: Reduce selector fragility and reconsider !important usage.The selector
.section.pt-0 .row.shadow.bg-white.p-5 > div:first-child imgis tightly coupled to the HTML structure (utility classes and element positions). This makes the CSS brittle; future markup changes could silently break the styling. Consider:
- Adding a dedicated CSS class to the target element in the HTML template (e.g.,
.cta-image) and referencing that instead.- Reviewing why
!importantis needed on both blocks; this often signals insufficient specificity in base styles. If the base styles use overqualified selectors or conflicting rules, consider refactoring those rather than layering!importantoverrides.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
assets/scss/_mixins.scss(1 hunks)config.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config.toml
🔇 Additional comments (1)
assets/scss/_mixins.scss (1)
162-164: Clarify image width value and remove contradictory comment.Line 163 contains a comment stating "320–360 works well" but the actual value applied is
180px. This is contradictory and may indicate a copy-paste error or incomplete refactoring. Verify that180pxis the intended value, and update the comment to reflect the actual constraint.
2 participants
This PR fixes the following issues mentioned in #251:
config.toml.Changes made in:
layouts/index.htmlassets/scss/templates/_main.scssconfig.tomlTested locally using
hugo server -D— layout looks consistent across all viewport widths ✅Before -
Desktop.2025.11.29.-.23.20.25.04.mp4
After -
Before -
After -
Summary by CodeRabbit
Chores
Style
✏️ Tip: You can customize this high-level summary in your review settings.