Add support for Option Type 'Buttons' kind and fix Variant Image count regression#20
Open
umeshravani wants to merge 4 commits into
Open
Add support for Option Type 'Buttons' kind and fix Variant Image count regression#20umeshravani wants to merge 4 commits into
umeshravani wants to merge 4 commits into
Conversation
Updated the condition to check for images using 'object.images.count' instead of 'object.image_count'. The Problem: The legacy image_count method was removed from the Spree::Variant model in 5.4 to optimize performance in favor of the new primary_media association. The Fix: Switched to object.images.count. This is a standard ActiveRecord call that resolves the "undefined method" crash while maintaining the logic that decides whether to show a secondary hover image.
Updated the variant picker layout to improve spacing and semantics. Changed class names and adjusted the structure for better accessibility. The Problem: The original file was binary—it only knew "Color Swatch" or "Dropdown." It ignored the new buttons kind. The Fix: Integrated a three-way switch based on option_type.kind. Native Integration: Replaced hardcoded Tailwind colors with Spree theme variables (text-default, bg-background, rounded-input). This ensures that if a user changes their theme settings in the Page Builder, the variant picker actually updates to match.
The Problem: High-UX friction. When a button was selected, it inherited global btn-primary styles which made it bold and uppercase, causing the button to "jump" in size and look misaligned. The Fix: Used Utility Overrides (!py-2 !px-4 !font-medium !normal-case). Result: The buttons now "hug" the text and maintain a perfectly consistent size regardless of whether they are selected or unselected.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds a single ERB comment with the template path to the featured image partial; no functional or interface changes are present. ChangesFeatured Image Template
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
Contributor
Author
|
Default First Load Preview: Selected Variant Preview: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
storefront/app/views/themes/default/spree/products/_featured_image.html.erb (2)
19-19: Remove commit-style inline note from the template.Line 19 looks like a temporary migration/debug note and should be removed to keep view code clean.
Suggested cleanup
- <%# FIXED LINE BELOW: Changed image_count to images.count %>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storefront/app/views/themes/default/spree/products/_featured_image.html.erb` at line 19, Remove the commit-style inline note from the ERB partial _featured_image.html.erb (the comment line "<%# FIXED LINE BELOW: Changed image_count to images.count %>") — simply delete that comment so the view contains only the actual template markup and helpers (e.g., the code using images.count or related helpers) without temporary migration/debug notes.
20-20: Simplify the image guard to avoid redundant association checks.At Line 20,
object.images.any? && object.images.count > 1is redundant. Checking if any images exist followed by checking if more than one exists is unnecessary. Useobject.images.size > 1instead for clarity and to avoid potential extra database queries.Proposed refactor
- <% if object.images.any? && object.images.count > 1 && object.secondary_image.present? && object.secondary_image.attached? %> + <% if object.images.size > 1 && object.secondary_image.present? && object.secondary_image.attached? %>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storefront/app/views/themes/default/spree/products/_featured_image.html.erb` at line 20, The conditional currently redundantly checks object.images.any? and object.images.count > 1; replace that compound check with a single size-based check to avoid extra queries—use object.images.size > 1 and keep the existing secondary_image check (object.secondary_image.present? && object.secondary_image.attached?) so the full guard becomes object.images.size > 1 && object.secondary_image.present? && object.secondary_image.attached?.storefront/app/views/themes/default/spree/products/_variant_options.html.erb (1)
41-52: Minor inconsistency in disabled opacity values.The buttons branch uses
opacity-40for disabled state (line 45), while the default dropdown branch usesopacity-50(line 63). Consider aligning these for visual consistency across option types.♻️ Optional: Align opacity values
- <%= 'opacity-40 cursor-not-allowed' if option_disabled %>"> + <%= 'opacity-50 cursor-not-allowed' if option_disabled %>">Alternatively, extract the disabled classes to a shared helper/constant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storefront/app/views/themes/default/spree/products/_variant_options.html.erb` around lines 41 - 52, The disabled opacity is inconsistent between the buttons branch (option_type.kind == 'buttons') which uses "opacity-40" and the dropdown branch which uses "opacity-50"; update the conditional for the label rendering that references option_disabled (and any corresponding class logic near radio_button_tag/selected_option) so both branches use the same opacity value (pick either 40 or 50) or extract the disabled classes into a shared helper/constant used by both branches to ensure visual consistency.storefront/app/views/themes/default/spree/products/_variant_picker.html.erb (1)
46-76: Accessibility:aria-haspopup="listbox"doesn't match dropdown content structure.The button declares
aria-haspopup="listbox"(line 52), but the dropdown menu (line 66) lacksrole="listbox", and the rendered options in_variant_options.html.erbdefault branch are radio inputs withoutrole="option"wrappers. This creates a semantic mismatch for assistive technologies.Consider either:
- Adding
role="listbox"to the menu container and wrapping options withrole="option", or- Removing
aria-haspopup="listbox"since the implementation uses radio inputs rather than a true listbox pattern♻️ Option 1: Add listbox role to dropdown menu
<div data-dropdown-target="menu" - class="hidden absolute top-12 left-0 z-50 w-full shadow-lg border border-default bg-background overflow-hidden rounded-input"> - <div class="max-h-60 overflow-y-auto"> + class="hidden absolute top-12 left-0 z-50 w-full shadow-lg border border-default bg-background overflow-hidden rounded-input" + role="listbox" + aria-label="<%= option_type.presentation %>"> + <div class="max-h-60 overflow-y-auto" role="presentation">Note: This would also require updating
_variant_options.html.erbdefault branch to wrap each option withrole="option".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storefront/app/views/themes/default/spree/products/_variant_picker.html.erb` around lines 46 - 76, The button currently declares aria-haspopup="listbox" but the dropdown (data-dropdown-target="menu") and the options (rendered by _variant_options.html.erb) don't follow listbox semantics; update the menu container (the element with data-dropdown-target="menu") to include role="listbox" and update _variant_options.html.erb default branch to wrap each option/radio with role="option" (or add role="option" to the radio's wrapper element) so ARIA roles match, keeping the button's aria-haspopup as-is; ensure each option has a unique id and, if you manage active selection, wire aria-activedescendant on the button/menu to that id for proper focus handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@storefront/app/views/themes/default/spree/products/_variant_picker.html.erb`:
- Line 9: Replace direct comparisons of option_type.kind == 'color_swatch' with
the predicate method option_type.color_swatch? to follow the codebase
convention; update the usages in the variant picker and variant options
templates (replace the occurrences of option_type.kind == 'color_swatch' with
option_type.color_swatch?), ensuring both conditional branches and any dependent
logic use the predicate method so behavior remains identical but consistent with
helpers like product_selected_option_for_option and
option_type_colors_preview_styles.
---
Nitpick comments:
In `@storefront/app/views/themes/default/spree/products/_featured_image.html.erb`:
- Line 19: Remove the commit-style inline note from the ERB partial
_featured_image.html.erb (the comment line "<%# FIXED LINE BELOW: Changed
image_count to images.count %>") — simply delete that comment so the view
contains only the actual template markup and helpers (e.g., the code using
images.count or related helpers) without temporary migration/debug notes.
- Line 20: The conditional currently redundantly checks object.images.any? and
object.images.count > 1; replace that compound check with a single size-based
check to avoid extra queries—use object.images.size > 1 and keep the existing
secondary_image check (object.secondary_image.present? &&
object.secondary_image.attached?) so the full guard becomes object.images.size >
1 && object.secondary_image.present? && object.secondary_image.attached?.
In
`@storefront/app/views/themes/default/spree/products/_variant_options.html.erb`:
- Around line 41-52: The disabled opacity is inconsistent between the buttons
branch (option_type.kind == 'buttons') which uses "opacity-40" and the dropdown
branch which uses "opacity-50"; update the conditional for the label rendering
that references option_disabled (and any corresponding class logic near
radio_button_tag/selected_option) so both branches use the same opacity value
(pick either 40 or 50) or extract the disabled classes into a shared
helper/constant used by both branches to ensure visual consistency.
In `@storefront/app/views/themes/default/spree/products/_variant_picker.html.erb`:
- Around line 46-76: The button currently declares aria-haspopup="listbox" but
the dropdown (data-dropdown-target="menu") and the options (rendered by
_variant_options.html.erb) don't follow listbox semantics; update the menu
container (the element with data-dropdown-target="menu") to include
role="listbox" and update _variant_options.html.erb default branch to wrap each
option/radio with role="option" (or add role="option" to the radio's wrapper
element) so ARIA roles match, keeping the button's aria-haspopup as-is; ensure
each option has a unique id and, if you manage active selection, wire
aria-activedescendant on the button/menu to that id for proper focus handling.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dad3de58-f793-4b6f-a44b-7f5ba3e5e46f
📒 Files selected for processing (3)
storefront/app/views/themes/default/spree/products/_featured_image.html.erbstorefront/app/views/themes/default/spree/products/_variant_options.html.erbstorefront/app/views/themes/default/spree/products/_variant_picker.html.erb
| <% selected_option = product_selected_option_for_option(option_type, product: product, selected_variant: selected_variant, options_param_name: options_param_name) %> | ||
| <% if option_type.color_swatch? %> | ||
|
|
||
| <% if option_type.kind == 'color_swatch' %> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that color_swatch? method is equivalent to kind == 'color_swatch'
# and check for any other usages of color_swatch? in the codebase
echo "=== Searching for color_swatch? method definition ==="
ast-grep --pattern 'def color_swatch?
$$$
end'
echo ""
echo "=== Searching for all color_swatch? usages ==="
rg -n 'color_swatch\?' --type erb --type ruby
echo ""
echo "=== Searching for kind comparisons ==="
rg -n "kind\s*==\s*['\"]color_swatch['\"]" --type erb --type rubyRepository: spree/spree-rails-storefront
Length of output: 1842
🏁 Script executed:
# Search for method definitions or where color_swatch? might be defined
echo "=== Searching for color_swatch in app/models ==="
find storefront/app/models -type f -name "*.rb" -exec rg -l "color_swatch" {} \;
echo ""
echo "=== Checking if there's a Gemfile with Spree dependencies ==="
head -20 Gemfile 2>/dev/null || head -20 storefront/Gemfile 2>/dev/null
echo ""
echo "=== Searching for any OptionType references ==="
rg -n "class.*OptionType\|def.*option_type" --type ruby -A 3 -B 1 | head -50
echo ""
echo "=== Looking for delegate or method_missing patterns ==="
rg -n "delegate\|alias_method\|define_method.*color" --type rubyRepository: spree/spree-rails-storefront
Length of output: 780
🏁 Script executed:
# Search in the Spree dependency to understand the OptionType model
echo "=== Checking if Spree is included as a gem/path ==="
cat storefront/Gemfile | grep -A 5 -B 5 "spree_opts\|'spree"
echo ""
echo "=== Look for any Spree version specifications ==="
rg -n "spree" storefront/Gemfile.lock -A 2 -B 1 | head -30
echo ""
echo "=== Check if there's any documentation or comments about the kind field ==="
rg -n "kind.*color_swatch\|color_swatch.*kind" --type erb --type ruby -B 2 -A 2
echo ""
echo "=== Search for schema or migration files that show the kind field ==="
find . -name "*.rb" -path "*/migrate/*" -o -name "schema.rb" | xargs rg -l "option_type\|option_kinds" 2>/dev/null | head -5Repository: spree/spree-rails-storefront
Length of output: 973
🏁 Script executed:
# Search for the git history of the variant_picker file to understand the change context
echo "=== Check git log for recent changes to _variant_picker.html.erb ==="
git log --oneline -10 -- storefront/app/views/themes/default/spree/products/_variant_picker.html.erb 2>/dev/null || echo "Git history not available"
echo ""
echo "=== Check if there's any PR or commit message explaining the change ==="
git log -p -1 -- storefront/app/views/themes/default/spree/products/_variant_picker.html.erb 2>/dev/null | head -50 || echo "Detailed log not available"
echo ""
echo "=== Verify the actual uses of color_swatch? vs kind == in context ==="
echo "File: _filters.html.erb"
rg -n -B 2 -A 2 "color_swatch\?" storefront/app/views/themes/default/spree/products/_filters.html.erb
echo ""
echo "File: _variant_options.html.erb (line 28)"
rg -n -B 2 -A 2 "kind.*color_swatch" storefront/app/views/themes/default/spree/products/_variant_options.html.erb
echo ""
echo "=== Check if option_type is the same object type in both cases ==="
echo "Looking at contexts where both methods might be called..."
rg -n "option_type\.kind\|option_type\.color_swatch" storefront/app/views/themes/default/spree/products/ -A 1 -B 1Repository: spree/spree-rails-storefront
Length of output: 997
🏁 Script executed:
# Since Spree is loaded from github, let's check if there's any inline documentation or comments
# about what color_swatch? does in the context of this repository
echo "=== Check all files that reference both kind and color_swatch in same context ==="
rg -n "option_type" storefront/app/views/themes/default/spree/products/_variant_picker.html.erb -B 5 -A 10 | head -40
echo ""
echo "=== Look at the full _variant_options file to see how option_type is used ==="
cat -n storefront/app/views/themes/default/spree/products/_variant_options.html.erb | head -50
echo ""
echo "=== Check if there are any comments or documentation about the kind field ==="
rg -n "kind\|color.swatch" storefront/app/views/themes/default/spree/products/_variant_picker.html.erb -B 3 -A 3Repository: spree/spree-rails-storefront
Length of output: 6319
🏁 Script executed:
# Let's check the variant_picker commit to understand the intent
echo "=== Full git diff for the variant_picker change ==="
git show a9831d0 -- storefront/app/views/themes/default/spree/products/_variant_picker.html.erb 2>/dev/null | head -100
echo ""
echo "=== Check if color_swatch? is consistently used in non-modified files ==="
rg -n "color_swatch\?" storefront/app/helpers/spree/products_helper.rb -B 1 -A 1
echo ""
echo "=== Summary: Files using direct kind comparison ==="
rg -l "kind.*==.*color_swatch\|kind.*==.*buttons" storefront/app/views --type erb
echo ""
echo "=== Summary: Files using color_swatch? method ==="
rg -l "color_swatch\?" storefront/app --type erb --type rubyRepository: spree/spree-rails-storefront
Length of output: 6584
Use color_swatch? method for consistency with rest of codebase.
The template uses direct option_type.kind == 'color_swatch' comparison (lines 9, 26), but this breaks the established pattern used throughout the codebase. Related helpers and finders consistently use the option_type.color_swatch? method instead:
product_selected_option_for_optionhelper (products_helper.rb:85-87)option_type_colors_preview_styleshelper (products_helper.rb:229)VariantFinderclass (variant_finder.rb:51, 67)- Other views: _filters.html.erb, _color_swatches.html.erb, _line_item_options.html.erb
Update lines 9 and 26 to use option_type.color_swatch? instead of option_type.kind == 'color_swatch' to maintain consistency with the Spree API pattern used throughout the application. Similarly, update _variant_options.html.erb lines 28 and 41.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@storefront/app/views/themes/default/spree/products/_variant_picker.html.erb`
at line 9, Replace direct comparisons of option_type.kind == 'color_swatch' with
the predicate method option_type.color_swatch? to follow the codebase
convention; update the usages in the variant picker and variant options
templates (replace the occurrences of option_type.kind == 'color_swatch' with
option_type.color_swatch?), ensuring both conditional branches and any dependent
logic use the predicate method so behavior remains identical but consistent with
helpers like product_selected_option_for_option and
option_type_colors_preview_styles.
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.
Summary of Changes
This PR updates the Spree Storefront theme to fully support the new Option Type Enhancement features introduced in Spree 5.4. It ensures that the storefront honors the kind rendering hint (Dropdown, Color Swatch, or Buttons) provided by the backend.
Problems Solved
Fixed 5.4 Core Regression: Resolved a NoMethodError in _featured_image.html.erb caused by the removal of the legacy image_count method from the Spree::Variant model. Switched to object.images.count for compatibility.
Native Theme Integration: Replaced hardcoded Tailwind utility classes in the variant picker with native Spree classes (btn-primary, rounded-input, border-default). This allows variant selectors to automatically inherit styles (colors, border-radius) set via the Spree Page Builder.
Buttons/Chips Support: Implemented a non-dropdown "Buttons" layout for option types.
Hugging Layout: The buttons are designed to "hug" the text content with consistent padding (py-2 px-4), preventing layout shifts when selecting options.
UI Stability: Applied specific CSS overrides (!font-medium, !normal-case) to ensure that selected buttons do not change size or text transformation, maintaining a stable UX.
A11y: Maintained accessibility standards using role="radiogroup" for buttons and role="listbox" for swatches.
Backwards Compatibility: Maintained the default "Dropdown" behavior for any option types where a specific kind is not defined.
Summary by CodeRabbit
New Features
Style
Refactor