Conversation
Contributor
Reviewer's GuideAdds optional bottom image support to the benefits card component, tightens conditional rendering for card content and actions, and updates SCSS so primary-background cards use secondary-outline button styling with a tailored hover state. Updated class diagram for the benefits card componentclassDiagram
class CMSFrontendComponent {
}
class BenefitsCard {
ImageFormField card_icon
CharField card_title
RichTextField card_content
CharField text_color
List child_plugin_instances
ImageFormField bottom_image
get_classes()
get_attributes()
}
CMSFrontendComponent <|-- BenefitsCard
Flow diagram for conditional rendering in benefits_card.htmlflowchart TD
A[Render benefits_card.html] --> B{instance.card_icon exists}
B -->|yes| C[Render icon wrapper and icon]
B -->|no| D[Skip icon wrapper]
C --> E{instance.card_title exists}
D --> E
E -->|yes| F[Render card_title wrapper and inline_field]
E -->|no| G[Skip title wrapper]
F --> H{instance.card_content stripped is nonempty}
G --> H
H -->|yes| I[Render benefits-card-body and inline_field card_content]
H -->|no| J[Skip card_content block]
I --> K{instance.child_plugin_instances exists}
J --> K
K -->|yes| L[Render mt-auto wrapper and childplugins]
K -->|no| M[Skip childplugins block]
L --> N{instance.bottom_image exists}
M --> N
N -->|yes| O[Render benefits-card-bottom-image wrapper and img]
N -->|no| P[Skip bottom image]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- You’ve wrapped the icon markup in an
{% if instance.card_icon %}but still unconditionally call{% add_css_for_icon instance.card_icon %}above; consider guarding that tag with the same condition to avoid issues whencard_iconis missing. - The selector
.benefits-card.bg-primary awill affect all links in the card, not just the primary button; consider scoping this styling to a more specific class (e.g..btn-primaryor a custom button class) to avoid unintended link styles. - The new bottom image renders with an empty
altattribute; if it conveys meaning, consider exposing an alt/description field on the model, or explicitly mark it as decorative (e.g.alt=""with appropriate role) to be intentional about accessibility.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You’ve wrapped the icon markup in an `{% if instance.card_icon %}` but still unconditionally call `{% add_css_for_icon instance.card_icon %}` above; consider guarding that tag with the same condition to avoid issues when `card_icon` is missing.
- The selector `.benefits-card.bg-primary a` will affect all links in the card, not just the primary button; consider scoping this styling to a more specific class (e.g. `.btn-primary` or a custom button class) to avoid unintended link styles.
- The new bottom image renders with an empty `alt` attribute; if it conveys meaning, consider exposing an alt/description field on the model, or explicitly mark it as decorative (e.g. `alt=""` with appropriate role) to be intentional about accessibility.
## Individual Comments
### Comment 1
<location path="cms_theme/templates/benefits/benefits_card.html" line_range="11-10" />
<code_context>
+ <i class="{{ instance.card_icon.iconClass }} display-1"></i>
+ </div>
+ {% endif %}
+ {% if instance.card_title %}
<div class="benefits-card-wrapper benefits-card-margin-bottom">
<h3 class="h4 fw-bold mb-4">{% inline_field instance "card_title" %}</h3>
</div>
+ {% endif %}
+ {% if instance.card_content|striptags %}
<div class="benefits-card-spacing flex-grow-1 benefits-card-body mt-auto">
</code_context>
<issue_to_address>
**issue:** Conditionally hiding the title block may disable inline editing when the field is empty.
Because the `inline_field` is only rendered when `instance.card_title` is truthy, an empty title removes the field and its inline controls, preventing editors from adding a title in place. Instead, keep the `inline_field` always rendered and handle the empty visual state with CSS, a placeholder, or a different condition that still allows inline editing.
</issue_to_address>
### Comment 2
<location path="cms_theme/templates/benefits/benefits_card.html" line_range="26-28" />
<code_context>
- </div>
+ </div>
+ {% endif %}
+ {% if instance.bottom_image %}
+ <div class="benefits-card-bottom-image">
+ <img src="{{ instance.bottom_image.url }}" alt="" class="w-100 d-block">
+ </div>
+ {% endif %}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Empty `alt` attribute on the bottom image may be problematic for accessibility depending on its purpose.
Empty `alt` should only be used for purely decorative images. If this image can ever convey information (e.g., product, meaningful logo, etc.), add a field for its description on `bottom_image` and use that value for `alt` so screen reader users don’t lose context.
Suggested implementation:
```
{% if instance.bottom_image %}
<div class="benefits-card-bottom-image">
<img
src="{{ instance.bottom_image.url }}"
alt="{{ instance.bottom_image.description|default_if_none:'' }}"
class="w-100 d-block"
>
</div>
{% endif %}
```
To fully support this in the codebase, you will also need to:
1. Add a `description` (or `alt_text`) field to whatever model backs `instance.bottom_image` (e.g. the image model or the plugin model) to store meaningful alt text.
2. Expose that field in the appropriate admin / CMS forms so editors can provide a description.
3. If `bottom_image` is meant to be decorative in some cases, document that editors can leave the description blank when it is purely decorative, in which case the template will render an empty `alt` attribute.
</issue_to_address>
### Comment 3
<location path="backend/static/scss/_benefits_panel.scss" line_range="28-33" />
<code_context>
+ }
+}
+
+.benefits-card.bg-primary {
+ a {
+ @extend .btn-outline-secondary;
+ color: var(--bs-secondary);
+
+ &:hover {
+ background-color: var(--bs-secondary);
+ color: white;
</code_context>
<issue_to_address>
**suggestion:** Using `@extend .btn-outline-secondary` inside `.benefits-card.bg-primary a` may cause CSS selector bloat or unexpected coupling.
`@extend` will inject `.benefits-card.bg-primary a` into every selector that uses `.btn-outline-secondary`, which can bloat the compiled CSS and tightly couple this link style to Bootstrap internals. To reuse the styles more safely, prefer a mixin or shared utility class instead of extending the Bootstrap button class directly.
Suggested implementation:
```
.benefits-card.bg-primary {
a {
// Use Bootstrap's mixin instead of extending the class to avoid selector bloat
@include button-outline-variant($secondary);
color: var(--bs-secondary);
&:hover {
background-color: var(--bs-secondary);
color: white;
}
}
}
```
If `button-outline-variant` is not yet in scope in this SCSS file, ensure that:
1. The main Bootstrap SCSS (or the file that defines the button mixins) is imported before `_benefits_panel.scss`.
2. If your project does not use Bootstrap’s SCSS mixins, you can instead create a local mixin (e.g. `@mixin benefits-outline-secondary-button { ... }`) that encapsulates the desired button styles, and replace `@include button-outline-variant($secondary);` with `@include benefits-outline-secondary-button;`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| <div class="mb-4 pb-1"> | ||
| <i class="{{ instance.card_icon.iconClass }} display-1"></i> | ||
| </div> | ||
| {% endif %} |
Contributor
There was a problem hiding this comment.
issue: Conditionally hiding the title block may disable inline editing when the field is empty.
Because the inline_field is only rendered when instance.card_title is truthy, an empty title removes the field and its inline controls, preventing editors from adding a title in place. Instead, keep the inline_field always rendered and handle the empty visual state with CSS, a placeholder, or a different condition that still allows inline editing.
Comment on lines
+26
to
+28
| {% if instance.bottom_image %} | ||
| <div class="benefits-card-bottom-image"> | ||
| <img src="{{ instance.bottom_image.url }}" alt="" class="w-100 d-block"> |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Empty alt attribute on the bottom image may be problematic for accessibility depending on its purpose.
Empty alt should only be used for purely decorative images. If this image can ever convey information (e.g., product, meaningful logo, etc.), add a field for its description on bottom_image and use that value for alt so screen reader users don’t lose context.
Suggested implementation:
{% if instance.bottom_image %}
<div class="benefits-card-bottom-image">
<img
src="{{ instance.bottom_image.url }}"
alt="{{ instance.bottom_image.description|default_if_none:'' }}"
class="w-100 d-block"
>
</div>
{% endif %}
To fully support this in the codebase, you will also need to:
- Add a
description(oralt_text) field to whatever model backsinstance.bottom_image(e.g. the image model or the plugin model) to store meaningful alt text. - Expose that field in the appropriate admin / CMS forms so editors can provide a description.
- If
bottom_imageis meant to be decorative in some cases, document that editors can leave the description blank when it is purely decorative, in which case the template will render an emptyaltattribute.
Comment on lines
+28
to
+33
| .benefits-card.bg-primary { | ||
| a { | ||
| @extend .btn-outline-secondary; | ||
| color: var(--bs-secondary); | ||
|
|
||
| &:hover { |
Contributor
There was a problem hiding this comment.
suggestion: Using @extend .btn-outline-secondary inside .benefits-card.bg-primary a may cause CSS selector bloat or unexpected coupling.
@extend will inject .benefits-card.bg-primary a into every selector that uses .btn-outline-secondary, which can bloat the compiled CSS and tightly couple this link style to Bootstrap internals. To reuse the styles more safely, prefer a mixin or shared utility class instead of extending the Bootstrap button class directly.
Suggested implementation:
.benefits-card.bg-primary {
a {
// Use Bootstrap's mixin instead of extending the class to avoid selector bloat
@include button-outline-variant($secondary);
color: var(--bs-secondary);
&:hover {
background-color: var(--bs-secondary);
color: white;
}
}
}
If button-outline-variant is not yet in scope in this SCSS file, ensure that:
- The main Bootstrap SCSS (or the file that defines the button mixins) is imported before
_benefits_panel.scss. - If your project does not use Bootstrap’s SCSS mixins, you can instead create a local mixin (e.g.
@mixin benefits-outline-secondary-button { ... }) that encapsulates the desired button styles, and replace@include button-outline-variant($secondary);with@include benefits-outline-secondary-button;.
…ry-background-need-secondary-border
fsbraun
changed the title
fsbraun
approved these changes
Mar 31, 2026
…ry-background-need-secondary-border
…ry-background-need-secondary-border
fsbraun
approved these changes
Apr 6, 2026
fsbraun
deleted the
feat/benefits-panel-primary-buttons-on-primary-background-need-secondary-border
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request adds support for an optional bottom image to the benefits card component, updates the card's template and styling to display this image, and improves the card's button styling for better visual consistency.
Enhancements to the benefits card component:
bottom_imagefield to the benefits card model, allowing editors to optionally specify an image to display at the bottom of the card.benefits_card.htmltemplate to render the bottom image if provided, and refactored conditional rendering for card icon, title, and content for better robustness.Styling and visual improvements:
.benefits-card-bottom-imageto ensure the bottom image is displayed full-width with appropriate border radius and overflow handling..benefits-card.bg-primaryto use secondary outline styling and improved hover state for better accessibility and consistency.Summary by Sourcery
Extend the benefits card component with optional bottom imagery and refine its conditional content rendering and visual styling.
New Features:
Enhancements: