-
Notifications
You must be signed in to change notification settings - Fork 202
[LUNA-3186]: Make marker prop optional in BpkPriceRange (breaking change option) #4133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[LUNA-3186]: Make marker prop optional in BpkPriceRange (breaking change option) #4133
Conversation
- Updated Props type to make marker optional - Added conditional rendering logic for marker and dot - Moved indicatorPercent calculation into useEffect for better performance - Used useCallback to memoize calcPercentage function - Added comprehensive test coverage (11 tests total) - Created NoMarkerWithLabels and NoMarkerWithoutLabels example components - Updated documentation with usage examples and props table - Verified functionality with Chrome DevTools and Storybook Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Removed useCallback in favour of simpler arrow function approach. All tests continue to pass. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Moved all conditional logic into dotClassName computation instead of duplicating checks at render time. This creates a single source of truth and makes the JSX more readable. Before: Check type for className, then marker && type && !showPriceIndicator for render After: Check all conditions for className, then just dotClassName for render All 11 tests continue to pass. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ake-marker-optional-in-price-range' into feature/LUNA-3186-make-marker-optional-in-price-range
…nges - Update unit tests to cover all 6 use cases with marker type field - Update accessibility tests for all use cases - Rename examples to reflect use cases clearly - Update stories with new example exports - Update README with new prop names and migration guide Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Visit https://backpack.github.io/storybook-prs/4133 to see this build running in a browser. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements breaking changes to improve the BpkPriceRange component API by making the marker prop optional and renaming showPriceIndicator to the more descriptive showPriceOnBoundaries. The changes add explicit control over marker display types ('bubble' or 'dot') through a new required type field in the marker object.
Changes:
- Renamed
showPriceIndicator→showPriceOnBoundaries(now required boolean) - Made
markerprop optional with new requiredtype: 'bubble' | 'dot'field - Updated component logic to handle optional marker and explicit type field
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/bpk-component-price-range/src/BpkPriceRange.tsx | Core component implementation with renamed prop, optional marker, and explicit type handling |
| packages/bpk-component-price-range/src/BpkPriceRange-test.tsx | Comprehensive test coverage for all 6 use cases with proper test organization |
| packages/bpk-component-price-range/src/accessibility-test.tsx | Accessibility tests covering all 6 use cases |
| packages/bpk-component-price-range/README.md | Updated documentation with new prop names, types, and examples for all use cases |
| examples/bpk-component-price-range/examples.tsx | Renamed and reorganized examples to match the 6 use cases with descriptive naming |
| examples/bpk-component-price-range/stories.tsx | Updated story exports with new example names matching use case structure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment was marked as outdated.
This comment was marked as outdated.
…Y_TYPES BREAKING CHANGE: Remove showPriceOnBoundaries prop - boundary visibility is now inferred from marker type (dot=hidden, bubble/none=shown). - Add MARKER_DISPLAY_TYPES constant to replace magic strings - Rename Props to BpkPriceRangeProps - Export MARKER_DISPLAY_TYPES and MarkerDisplayType from package - Update tests, examples, and documentation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
|
Hi Jack Waller (@Jack-Waller), have you tried to generate a codemod for this breaking change? If we could just run a script for our consumers it's not that big of a breaking change I reckon. Especially since usages are relatively low. |
Hi Gert-Jan Vercauteren (@gert-janvercauteren), great suggestion! We're evaluating weather we want to proceed with this PR now: I think a codemod should be straightforward for this change if we do 🙂 I'm in the process of writing a decision document for this change if you're interested (still WIP). |
- Add unit test for bubble marker rendering when type is omitted - Add accessibility test for default marker type scenario - Add example demonstrating default type behaviour - Update README to document type as optional with BUBBLE default 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…-breaking-changes
|
Visit https://backpack.github.io/storybook-prs/4133 to see this build running in a browser. |
Summary
This PR introduces breaking changes to
BpkPriceRangeto improve the component API and enable new use cases for price bands display.Key Changes
markerprop optional: The component can now display only the price range bars without a markerMARKER_DISPLAY_TYPESconstant: New exported constant withBUBBLEandDOToptions to control marker display styleshowPriceOnBoundariesprop: Boundary price visibility is now automatically determined by the marker typeBUBBLE: Whenmarkeris provided without atype, it defaults toBUBBLEbehaviourBoundary Price Visibility Rules
MARKER_DISPLAY_TYPES.BUBBLEMARKER_DISPLAY_TYPES.DOTChecklist
[Clover-123][BpkButton] Updating the colourREADME.md(If you have created a new component)README.mdMigration Guide
Breaking Changes
showPriceOnBoundariesprop removedshowPriceOnBoundaries={false}to hide boundary pricesmarker={{ type: MARKER_DISPLAY_TYPES.DOT, ... }}for compact display without boundariesmarker.typeis now required when using marker with dot styleMARKER_DISPLAY_TYPES.BUBBLEorMARKER_DISPLAY_TYPES.DOTmarker.typedefaults toBUBBLEwhen omittedmarkerwithout atype, it will default toMARKER_DISPLAY_TYPES.BUBBLEMigration Examples
Before (with boundaries hidden):
After (using dot marker):
No change to UI:

Before (with boundaries shown):
After (using bubble marker - default):
No change to UI:

Without marker (new use case):
References
🤖 Generated with Claude Code