Story 2432: Hero Section Refactor#2476
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR updates v3 hero image context, hero markup, and responsive CSS for homepage, community, and release pages. It also removes several page-specific header overlay rules and adjusts library button typography. ChangesHero assets and responsive layout refactor
Library button typography
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Stylelint (17.13.0)static/css/v3/heros.cssError: ENOENT: no such file or directory, open '/.stylelintrc.json' 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@static/css/v3/heros.css`:
- Around line 150-153: The CSS rule for .hero-home--no-image contains redundant
background declarations causing stylelint's
declaration-block-no-shorthand-property-overrides: remove the explicit
background-image: none declaration and keep the shorthand background: none (or
vice versa if you prefer the longhand), and apply the same change to the other
occurrence (the similar block at lines around 499-502) so no shorthand
immediately overrides a longhand.
In `@templates/v3/includes/_hero_home.html`:
- Line 16: The inline comment text for hero_image_url_mobile contains an
unescaped '<' in the string "<=767px" which triggers the HTMLHint
spec-char-escape rule; update the comment in
templates/v3/includes/_hero_home.html (the hero_image_url_mobile description) to
escape the character (e.g. replace "<" with "<") so the comment reads
"<=767px" and clears the linter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68055739-52df-48cc-a26b-05817f340585
📒 Files selected for processing (16)
ak/views.pycore/mock_data.pycore/views.pystatic/css/v3/auth-page.cssstatic/css/v3/community.cssstatic/css/v3/header.cssstatic/css/v3/heros.cssstatic/css/v3/library-subpage.cssstatic/css/v3/release-detail.cssstatic/css/v3/v3-homepage.csstemplates/v3/community.htmltemplates/v3/examples/_v3_example_section.htmltemplates/v3/homepage.htmltemplates/v3/includes/_hero_home.htmltemplates/v3/includes/_hero_library.htmltemplates/v3/release_detail.html
💤 Files with no reviewable changes (3)
- static/css/v3/auth-page.css
- static/css/v3/library-subpage.css
- static/css/v3/v3-homepage.css
|
@rbbeeston here's the preview url https://hero-refactor-2432.ngrok.app/ |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
templates/v3/includes/_hero_home.html (1)
43-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove
loading="lazy"from both hero images (light and dark). Hero images are LCP elements and appear above the fold. Lazy loading defers their load and degrades Web Vitals. Useloading="eager"or omit the attribute on both the light (line 43) and dark (line 48)<img>tags.⚡ Proposed fix
- <img alt="" class="hero-home__img hero-home__img--light" src="{{ hero_light }}" loading="lazy" decoding="async"> + <img alt="" class="hero-home__img hero-home__img--light" src="{{ hero_light }}" decoding="async">- <img alt="" class="hero-home__img hero-home__img--dark" src="{{ hero_dark }}" loading="lazy" decoding="async"> + <img alt="" class="hero-home__img hero-home__img--dark" src="{{ hero_dark }}" decoding="async">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@templates/v3/includes/_hero_home.html` at line 43, Remove lazy loading from the hero images: locate the two <img> tags with classes hero-home__img hero-home__img--light and hero-home__img hero-home__img--dark and either delete the loading="lazy" attribute or change it to loading="eager" so the LCP hero images load immediately (apply the same change to both light and dark hero <img> tags).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@templates/v3/includes/_hero_home.html`:
- Line 43: Remove lazy loading from the hero images: locate the two <img> tags
with classes hero-home__img hero-home__img--light and hero-home__img
hero-home__img--dark and either delete the loading="lazy" attribute or change it
to loading="eager" so the LCP hero images load immediately (apply the same
change to both light and dark hero <img> tags).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bbbc25be-c786-4d8f-9d6d-8fdf27c479ca
📒 Files selected for processing (3)
static/css/v3/heros.csstemplates/v3/includes/_hero_home.htmltemplates/v3/includes/_hero_library.html
💤 Files with no reviewable changes (1)
- static/css/v3/heros.css
🚧 Files skipped from review as they are similar to previous changes (1)
- templates/v3/includes/_hero_library.html
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
templates/v3/includes/_hero_library.html (1)
29-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove stale
is_flagship_libvariable from the template contract docs.The refactor removed the flagship-specific title styling path, but the comment block still advertises
is_flagship_lib; this can mislead callers into passing dead input.Suggested doc cleanup
- is_flagship_lib (optional) — Boolean value stating whether or not this is the hero for a flagship library. If it isn't, styles are a bit different.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@templates/v3/includes/_hero_library.html` around lines 29 - 60, The template documentation or comment block at the top of _hero_library.html still references the `is_flagship_lib` variable in the template contract, but this variable is no longer used after the refactor that removed the flagship-specific title styling. Find and remove the reference to `is_flagship_lib` from the documentation or comment block to prevent callers from passing this dead input variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@static/css/v3/heros.css`:
- Around line 667-670: The .hero__title mobile rule is using the --font-size-3xl
token, which is the 64px desktop heading tier, but mobile should use the 32px
tier instead. Replace the font-size property value in the .hero__title selector
with the project's 32px font size token (check the design system tokens to find
the correct token name, such as --font-size-2xl or similar) to align with the
documented mobile design ladder.
- Around line 65-81: The `--hero-bg-community-size` CSS custom property is being
defined on `:root` in the tablet and mobile media queries, which causes all hero
elements to inherit this community-specific sizing instead of just the community
hero variant. Move the `--hero-bg-community-size` variable definition from the
`:root` selector to `.hero--community` selector in both the `@media` (max-width:
1279px) and `@media` (max-width: 767px) blocks, so that the community-specific
background sizing only applies to elements with the `.hero--community` class
while the base `.hero` class maintains its default `cover` sizing.
---
Nitpick comments:
In `@templates/v3/includes/_hero_library.html`:
- Around line 29-60: The template documentation or comment block at the top of
_hero_library.html still references the `is_flagship_lib` variable in the
template contract, but this variable is no longer used after the refactor that
removed the flagship-specific title styling. Find and remove the reference to
`is_flagship_lib` from the documentation or comment block to prevent callers
from passing this dead input variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 972285c9-e6a9-4760-b3d2-9146d63b541b
📒 Files selected for processing (10)
ak/views.pycore/views.pystatic/css/v3/community.cssstatic/css/v3/heros.csstemplates/v3/community.htmltemplates/v3/homepage.htmltemplates/v3/includes/_hero_home.htmltemplates/v3/includes/_hero_library.htmltemplates/v3/release_detail.htmlversions/views.py
✅ Files skipped from review due to trivial changes (1)
- templates/v3/community.html
🚧 Files skipped from review as they are similar to previous changes (4)
- templates/v3/release_detail.html
- templates/v3/homepage.html
- core/views.py
- ak/views.py
| .hero__title { | ||
| font-size: var(--font-size-3xl); | ||
| } | ||
| letter-spacing: 0px; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the 32px mobile heading tier.
The mobile rule switches .hero__title back to --font-size-3xl, the same token used for the documented 64px desktop tier, so phone headings won’t follow the stated 32px mobile ladder.
Proposed fix
.hero__title {
- font-size: var(--font-size-3xl);
+ font-size: var(--font-size-xl);
letter-spacing: 0px;
}Use the project’s existing 32px token if it is named differently.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .hero__title { | |
| font-size: var(--font-size-3xl); | |
| } | |
| letter-spacing: 0px; | |
| } | |
| .hero__title { | |
| font-size: var(--font-size-xl); | |
| letter-spacing: 0px; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@static/css/v3/heros.css` around lines 667 - 670, The .hero__title mobile rule
is using the --font-size-3xl token, which is the 64px desktop heading tier, but
mobile should use the 32px tier instead. Replace the font-size property value in
the .hero__title selector with the project's 32px font size token (check the
design system tokens to find the correct token name, such as --font-size-2xl or
similar) to align with the documented mobile design ladder.
Okay, so how does this look with Home and Community sharing same styles. No unique positioning |
Merges the home, community, and library/release heroes into a single .hero base with per-page modifiers and one heading style, splits the mascot foreground from the background so each scales independently across breakpoints, and drives all the responsive sizing through tokens set once per tier. Hero image URLs move out of the templates and into the page views, and the duplicated absolute-header rules collapse into one body.v3 .header rule. Also brings the hero link buttons' line-height and letter-spacing to the Figma spec, which closes the remaining open item on boostorg#2367.
julioest
force-pushed
the
2432-hero-section-refactor
branch
from
33ab24f to
5e18c9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/views.py (1)
520-521: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid re-entering
get_context_data()fromget_v3_context_data().
V3Mixin.render_v3_response()already wrapsget_v3_context_data()in a finalget_context_data()call, so this does the full context build twice per request, includingMailingListCardMixin's extra work.Proposed fix
def get_v3_context_data(self, **kwargs): - ctx = self.get_context_data(**kwargs) + ctx = super().get_v3_context_data(**kwargs) ctx["learn_card_data"] = [🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/views.py` around lines 520 - 521, Avoid calling get_context_data() inside get_v3_context_data(), since V3Mixin.render_v3_response() already performs the final context build and this causes the context to be assembled twice. Update get_v3_context_data() to work only with the v3-specific additions, and let render_v3_response() handle the full get_context_data() merge; use get_v3_context_data and V3Mixin.render_v3_response as the key symbols when refactoring.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@templates/v3/includes/_hero_home.html`:
- Around line 40-48: The hero rendering in _hero_home.html currently always
emits the light <img> inside the hero picture block, so when only
hero_image_url_dark is provided hero_light becomes empty and the light image
gets an empty src. Update the template around the hero_light/hero_dark with
block so the light <img> is only rendered when a valid hero_light value exists,
while still rendering the dark picture when hero_dark is present; use the
existing hero_image_url_mobile and hero_dark symbols to keep the structure
consistent.
In `@templates/v3/includes/_hero_library.html`:
- Around line 83-91: The shared hero rendering in _hero_library.html still
assumes a light image is always available, so dark-only inputs leave the light
<img> empty. Update the hero handling around the hero_light/hero_dark assignment
and the picture block so the light image is only rendered when a light source
exists, or otherwise falls back to the dark image; keep the same behavior
aligned with _hero_home.html and use the existing hero_image_url_light,
hero_image_url_dark, and hero_image_url_mobile symbols to locate the fix.
---
Nitpick comments:
In `@core/views.py`:
- Around line 520-521: Avoid calling get_context_data() inside
get_v3_context_data(), since V3Mixin.render_v3_response() already performs the
final context build and this causes the context to be assembled twice. Update
get_v3_context_data() to work only with the v3-specific additions, and let
render_v3_response() handle the full get_context_data() merge; use
get_v3_context_data and V3Mixin.render_v3_response as the key symbols when
refactoring.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4306ab80-0f73-4fdc-af78-7169c4a74a1f
📒 Files selected for processing (18)
ak/views.pycore/mock_data.pycore/views.pystatic/css/v3/auth-page.cssstatic/css/v3/buttons.cssstatic/css/v3/community.cssstatic/css/v3/header.cssstatic/css/v3/heros.cssstatic/css/v3/library-subpage.cssstatic/css/v3/release-detail.cssstatic/css/v3/v3-homepage.csstemplates/v3/community.htmltemplates/v3/examples/_v3_example_section.htmltemplates/v3/homepage.htmltemplates/v3/includes/_hero_home.htmltemplates/v3/includes/_hero_library.htmltemplates/v3/release_detail.htmlversions/views.py
💤 Files with no reviewable changes (3)
- static/css/v3/library-subpage.css
- static/css/v3/auth-page.css
- static/css/v3/v3-homepage.css
✅ Files skipped from review due to trivial changes (1)
- templates/v3/release_detail.html
🚧 Files skipped from review as they are similar to previous changes (8)
- templates/v3/homepage.html
- static/css/v3/community.css
- core/mock_data.py
- versions/views.py
- templates/v3/community.html
- ak/views.py
- static/css/v3/header.css
- static/css/v3/heros.css
Scopes the community background sizing to .hero--community so library and release heroes keep the base cover instead of inheriting the community art's per-breakpoint sizing.
Also guards the light hero <picture> with an hero_light check so a dark-only config can't emit an empty image src. This uses {% if hero_light %} rather than the suggested chained default, because the community and release includes use 'only' and a missing variable used as a filter argument raises VariableDoesNotExist.
1 participant
Issue: #2432
Refactors the v3 hero on the homepage and community pages into one reusable component. Each page used to style its hero separately; now there's a single
.herowith small per-page modifiers, a shared full-bleed mascot frame, and all the responsive sizing driven by tokens. Hero image URLs moved out of the templates and into the views.Changes
.herobase with--home/--community/--releasemodifiers, and one heading style instead of three. Page stylesheets no longer reach into hero internals.<picture>, and a community hero that reuses the home-style frame over a taller room background.body.v3 .headerrule with a padding reservation, removing the copies from the auth, library subpage, and release detail stylesheets.Risks
v3class.Testing
Screenshots
Homepage
Community
Self-review Checklist
Frontend
Summary by CodeRabbit
New Features
Improvements
Style