Skip to content

Conversation

@nhobes
Copy link
Contributor

@nhobes nhobes commented Jan 14, 2026

Core Implementation

  • Added dual range slider component (type="range-dual") using Alpine.js for client-side interactivity
  • Implemented in input.ex, field.ex, and form.ex for consistent API across component variants
  • Works seamlessly in both LiveView and controller-rendered (dead) views

Technical Improvements

  • Added phx-update="ignore" to prevent LiveView/Alpine.js DOM conflicts
  • Implemented $nextTick() wrapper for proper Alpine initialization timing
  • Added Jason.encode!() for safe JavaScript string interpolation (prevents injection)
  • Added division by zero guard clauses when range_min == range_max
  • Proper error state handling and display with field wrapper integration

Features

  • Support for custom formatters (e.g., currency: format_value={&("$#{&1}")})
  • Configurable min/max labels, step values, and range bounds
  • Real-time visual feedback with sliding track indicator
  • Form submission with standard name attributes (no special handling required)
  • Accessible markup with proper ARIA attributes

@greptile-apps
Copy link

greptile-apps bot commented Jan 14, 2026

Greptile Summary

This PR successfully adds a dual range slider component (type="range-dual") with Alpine.js for interactive client-side updates. The implementation includes proper division by zero guards, XSS prevention via Jason.encode!(), and phx-update="ignore" to prevent LiveView/Alpine.js DOM conflicts.

Key Changes:

  • Added dual range slider implementation in input.ex, field.ex, and form.ex with consistent API
  • Implemented Alpine.js integration with $nextTick() for proper initialization timing
  • Added comprehensive test coverage including edge cases (range_min == range_max)
  • Created polished CSS styling with hover/focus states, dark mode support, and accessibility features
  • Updated documentation to clarify Alpine.js requirement for the new component
  • Added custom formatter support for displaying values (e.g., currency formatting)

Technical Quality:

  • Division by zero guards properly implemented in both Elixir (calculate_slider_position/3) and JavaScript (updateRange())
  • XSS protection through Jason.encode!() for safe JavaScript string interpolation
  • Proper LiveView/Alpine.js boundary management with phx-update="ignore"
  • Good test coverage for core functionality and edge cases
  • Clean separation of concerns between rendering logic and interactive behavior

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The implementation demonstrates excellent technical quality with proper edge case handling, security considerations, and comprehensive test coverage. Division by zero guards are correctly implemented in both Elixir and JavaScript, XSS prevention is in place via Jason.encode!(), and the LiveView/Alpine.js boundary is properly managed. The code follows existing patterns in the codebase, includes thorough tests for edge cases, and the changes are well-contained to the new feature without breaking existing functionality.
  • No files require special attention

Important Files Changed

Filename Overview
lib/petal_components/input.ex Added dual range slider with proper division by zero guards and Alpine.js integration
lib/petal_components/field.ex Added field wrapper for dual range slider with consistent formatter API
test/petal/input_test.exs Added comprehensive tests for dual range slider including edge cases and formatters
assets/default.css Added comprehensive slider styling with hover/focus states and dark mode support

Sequence Diagram

sequenceDiagram
    participant User
    participant Browser
    participant LiveView
    participant Input/Field Component
    participant Alpine.js

    User->>Browser: Load page with dual range slider
    Browser->>LiveView: Request page
    LiveView->>Input/Field Component: Render range-dual type
    Input/Field Component->>Input/Field Component: calculate_slider_position() for initial values
    Input/Field Component->>Input/Field Component: Jason.encode!() for safe JS interpolation
    Input/Field Component->>Browser: Return HTML with x-data, phx-update="ignore"
    Browser->>Alpine.js: Initialize component ($nextTick)
    Alpine.js->>Alpine.js: init() -> updateRange()
    Alpine.js->>Browser: Calculate and render slider track positions

    User->>Browser: Drag min/max slider thumb
    Browser->>Alpine.js: @input event triggered
    Alpine.js->>Alpine.js: updateMinValue/updateMaxValue()
    Alpine.js->>Alpine.js: Validate (min <= max)
    Alpine.js->>Alpine.js: updateRange() - recalculate positions
    Alpine.js->>Browser: Update visual track position
    Alpine.js->>Browser: Update displayed values (x-text)
    
    User->>Browser: Submit form
    Browser->>LiveView: POST with min/max field values
    LiveView->>LiveView: Process form data
    
    Note over LiveView,Alpine.js: phx-update="ignore" prevents LiveView from overwriting Alpine state
Loading

@greptile-apps
Copy link

greptile-apps bot commented Jan 14, 2026

Greptile's behavior is changing!

From now on, if a review finishes with no comments, we will not post an additional "statistics" comment to confirm that our review found nothing to comment on. However, you can confirm that we reviewed your changes in the status check section.

This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR".

nhobes and others added 9 commits January 14, 2026 13:46
- Prevents ArithmeticError when range_min equals range_max
- Matches the fix already applied to field.ex
- Ensures dual range slider edge case test passes
- Replace incorrect pc_used_input? implementation with Phoenix.Component.used_input?
- Update dual range slider error test to include field in params
- All tests now passing

Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
@nhobes nhobes force-pushed the dual-range-slider branch from ca64f7d to 8cf8160 Compare January 14, 2026 02:52
@codecov
Copy link

codecov bot commented Jan 14, 2026

Codecov Report

❌ Patch coverage is 91.34615% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.00%. Comparing base (7970971) to head (42683c5).

Files with missing lines Patch % Lines
lib/petal_components/field.ex 93.68% 6 Missing ⚠️
lib/petal_components/form.ex 88.88% 6 Missing ⚠️
lib/petal_components/input.ex 89.47% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #467      +/-   ##
==========================================
+ Coverage   89.73%   90.00%   +0.27%     
==========================================
  Files          37       37              
  Lines        1607     1801     +194     
==========================================
+ Hits         1442     1621     +179     
- Misses        165      180      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Add 23 new tests covering form.ex, input.ex, and field.ex implementations
- Fix missing attribute definitions in form.ex for dual range support
- Add division-by-zero guard in calculate_slider_position helper
- Test coverage for nil values, negative ranges, custom steps, formatters
- Test coverage for both range-dual and range-numeric types
- All 323 tests passing

This significantly improves codecov coverage for the dual range slider feature.

Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
@nhobes nhobes requested a review from mplatts January 14, 2026 04:47
max_value = @max_field.value || @range_max

# Get formatter function
formatter = assigns[:formatter] || (&to_string/1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use assigns in the template. assign in the live view (assign(socket, formatter: blah))

<.form_label form={@form} field={@field} label={@label} class={@label_class} />
<%
# Set default values if nil
min_value = @min_field.value || @range_min
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think it's good to assign variables in the template. maybe do it in the live view

Comment on lines +964 to +985
# Helper functions for dual range slider
defp calculate_slider_position(nil, _range_min, _range_max), do: 0

# Guard against division by zero when range_min == range_max
defp calculate_slider_position(_value, range_min, range_max)
when range_min == range_max,
do: 0

defp calculate_slider_position(value, range_min, range_max) when is_integer(value) do
round((value - range_min) / (range_max - range_min) * 100)
end

defp calculate_slider_position(value, range_min, range_max) do
value =
case value do
v when is_binary(v) -> String.to_integer(v)
v when is_integer(v) -> v
_ -> range_min
end

round((value - range_min) / (range_max - range_min) * 100)
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put all this in a new component .range_dual

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants