Skip to content

[LUNA-3186]: Make marker prop optional in BpkPriceRange#4158

Merged
Vincent Liu (xiaogliu) merged 6 commits intomainfrom
feat/LUNA-3186-optional-marker-expand-phase
Jan 30, 2026
Merged

[LUNA-3186]: Make marker prop optional in BpkPriceRange#4158
Vincent Liu (xiaogliu) merged 6 commits intomainfrom
feat/LUNA-3186-optional-marker-expand-phase

Conversation

@Jack-Waller
Copy link
Copy Markdown
Member

@Jack-Waller Jack Waller (Jack-Waller) commented Jan 27, 2026

Summary

Adds the ability to hide the marker in the BpkPriceRange component.

image

Storybook

Also expands the API, by introducing an optional type property of the now optional marker property. This intends to replace the already-optional showPriceIndicator property, making it clear which marker style we're getting.

This PR therefore also deprecates the showPriceIndicator property. This component accepts any combination of the new marker.type and showPriceIndicator properties. The default behaviour (neither property provided) remains the same. Otherwise, the value of marker.type will override the value of showPriceIndicator.

We expect a follow-up PR corresponding to a major change, where we will remove showPriceIndicator.

This corresponds to Option 4 from the decision document.

Key Changes

  1. Made marker prop optional - The component can now be used without a marker
  2. Deprecated showPriceIndicator prop - Marked with JSDoc @deprecated tag
  3. Added marker.type API - New MARKER_DISPLAY_TYPES constant with BUBBLE and DOT options
  4. Updated type definitions - Added MarkerDisplayType and restructured types for clarity
  5. Created migration guide - Comprehensive documentation in docs/migrating-from-showPriceIndicator.md

Motivation

The previous showPriceIndicator prop had confusing semantics:

  • It wasn't clear what was being shown/hidden (boundary prices, not the marker itself)
  • It coupled marker display type with boundary price visibility
  • It didn't allow for different marker display styles

The new marker.type API provides:

  • Clearer intent: MARKER_DISPLAY_TYPES.BUBBLE vs MARKER_DISPLAY_TYPES.DOT clearly describes the marker appearance
  • Better flexibility: The marker type naturally determines boundary price visibility
  • Explicit behaviour: BUBBLE shows boundaries, DOT hides them
  • One less edge case: We don't need to define a type for an undefined marker.

Migration Path (Expand-Migrate-Contract)

This is the expand phase:

  • ✅ New API added (marker.type with MARKER_DISPLAY_TYPES)
  • ✅ Old API still works (showPriceIndicator)
  • ✅ Deprecation warning added
  • ⏳ Next: Migrate consumers to new API (no change needed if using default values already)
  • ⏳ Future: Remove deprecated prop in major version

Changes

  • Component (BpkPriceRange.tsx):

    • Made marker prop optional
    • Added marker.type support with new MARKER_DISPLAY_TYPES
    • Deprecated showPriceIndicator with JSDoc annotation
    • Implemented backward compatibility logic
  • Type definitions (common-types.ts):

    • Added MarkerDisplayType type
    • Added MARKER_DISPLAY_TYPES constant
    • Exported new type
  • Tests:

    • Updated all existing tests to use new API
    • Added tests for optional marker behaviour
    • Verified backward compatibility with showPriceIndicator
    • Updated accessibility tests
  • Documentation:

    • Created comprehensive migration guide (docs/migrating-from-showPriceIndicator.md)
    • Updated component README with new examples
    • Added usage examples for both APIs
  • Examples & Stories:

    • Updated Storybook stories to demonstrate new API
    • Added examples showing MARKER_DISPLAY_TYPES.DOT usage
    • Kept examples demonstrating backward compatibility

Testing

All existing tests pass with backward compatibility maintained. New tests added for:

  • Optional marker prop
  • MARKER_DISPLAY_TYPES.BUBBLE behaviour
  • MARKER_DISPLAY_TYPES.DOT behaviour
  • Backward compatibility with showPriceIndicator

Accessibility

No accessibility changes - the component maintains the same ARIA attributes and keyboard navigation:

  • Keyboard navigation unchanged
  • Zoom functionality unchanged
  • Screen reader behaviour unchanged

References


Remember to include the following changes:

  • Ensure the PR title includes the name of the component
  • Component README.md updated
  • Tests updated
  • Accessibility tests updated
  • Storybook examples created/updated
  • Migration guide added

🤖 Generated with Claude Code

- Deprecate showPriceIndicator prop in favour of marker.type API
- Add MARKER_DISPLAY_TYPES constant (BUBBLE and DOT)
- Make marker prop optional to support usage without a marker
- Update type definitions with MarkerDisplayType
- Add migration guide documentation
- Update tests to cover new optional marker behaviour
- Update examples and stories to demonstrate new API

This is the expand phase of an expand-migrate-contract migration.
The showPriceIndicator prop is now deprecated but still functional
to maintain backward compatibility.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@Jack-Waller Jack Waller (Jack-Waller) added the minor Non breaking change label Jan 27, 2026
@skyscanner-backpack-bot

This comment was marked as outdated.

@skyscanner-backpack-bot
Copy link
Copy Markdown

skyscanner-backpack-bot Bot commented Jan 27, 2026

Warnings
⚠️

Package source files (e.g. packages/package-name/src/Component.js) were updated, but snapshots weren't. Have you checked that the tests still pass?

Browser support

If this is a visual change, make sure you've tested it in multiple browsers.

Generated by 🚫 dangerJS against 2b5b320

@Jack-Waller Jack Waller (Jack-Waller) changed the title [LUNA-3186]: Make marker prop optional in BpkPriceRange (expand - migrate - contract option) WIP [LUNA-3186]: Make marker prop optional in BpkPriceRange (expand - migrate - contract option) Jan 27, 2026
*/
showPriceIndicator?: boolean;
marker: Marker;
marker?: MarkerPriceRangePosition;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will an error occur on the client side if the type is not updated after the type has been changed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No 🙂 Previously, the only type exported was BpkPriceRangeProps, the Marker type was never directly exported. While consumers can use the Marker type, they can only ever access it via indexing through the marker property (e.g. BpkPriceRangeProps['marker']). As MarkerPriceRangePosition is a superset of the original Marker, and we're not changing the name of the marker property, if a consumer fetched the old type of Marker by indexing through marker, when updating to the new minor version of backpack, the index will now point to MarkerPriceRangePosition, and will compile without errors.

Even if the consumer defined a new type themselves representing a Marker, this could still be passed safely (as the new MarkerPriceRangePosition is a superset of the old Marker). Testing this locally shows no errors 🙂

Image

@skyscanner-backpack-bot

This comment was marked as outdated.

@skyscanner-backpack-bot

This comment was marked as outdated.

@skyscanner-backpack-bot
Copy link
Copy Markdown

Visit https://backpack.github.io/storybook-prs/4158 to see this build running in a browser.

} from './src/BpkPriceRange';

export type { BpkPriceRangeProps };
export default BpkPriceRange;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

SonarQube complained about this:

Image

Comment on lines +147 to +151
const defaultMarkerType = showPriceIndicator ?? true ? MARKER_DISPLAY_TYPES.BUBBLE : MARKER_DISPLAY_TYPES.DOT;
const markerType = marker?.type || defaultMarkerType;
const shouldShowMarker = !!marker;
const shouldShowBubble = shouldShowMarker && markerType === MARKER_DISPLAY_TYPES.BUBBLE;
const shouldShowDot = shouldShowMarker && markerType === MARKER_DISPLAY_TYPES.DOT;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If marker.type is set, use it. Otherwise, default to bubble unless showPriceIndicator is explicitly false, in which case show a dot.

Details All of the below logic formulas evaluate to `true`.

Here's a ChatGPT summary of the below

  • ->: implication
  • =: equality
  • &: conjunction (and)
  1. (marker.type = bubble & showPriceIndicator = undefined) -> (shouldShowBubble = true) & (shouldShowDot = false)
  2. (marker.type = bubble & showPriceIndicator = true) -> (shouldShowBubble = true) & (shouldShowDot = false)
  3. (marker.type = bubble & showPriceIndicator = false) -> (shouldShowBubble = true) & (shouldShowDot = false)
  4. ((marker.type = dot & showPriceIndicator = undefined) -> (shouldShowBubble = false) & (shouldShowDot = true)
  5. ((marker.type = dot & showPriceIndicator = true) -> (shouldShowBubble = false) & (shouldShowDot = true)
  6. ((marker.type = dot & showPriceIndicator = false) -> (shouldShowBubble = false) & (shouldShowDot = true)
  7. ((marker.type = undefined & showPriceIndicator = undefined) -> (shouldShowBubble = true) & (shouldShowDot = false)
  8. ((marker.type = undefined & showPriceIndicator = true) -> (shouldShowBubble = true) & (shouldShowDot = false)
  9. ((marker.type = undefined & showPriceIndicator = false) -> (shouldShowBubble = false) & (shouldShowDot = true)

@Jack-Waller Jack Waller (Jack-Waller) marked this pull request as ready for review January 28, 2026 14:58
Copilot AI review requested due to automatic review settings January 28, 2026 14:58
@Jack-Waller Jack Waller (Jack-Waller) changed the title WIP [LUNA-3186]: Make marker prop optional in BpkPriceRange (expand - migrate - contract option) [LUNA-3186]: Make marker prop optional in BpkPriceRange Jan 28, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

}) => <div style={{ width: isLarge ? '15rem' : '8.75rem' }}>{children}</div>;

const SmallerLowPriceRangeExample = () => (
// Use case 1: Dot marker (boundaries hidden)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit, we expect to understand the code through the code itself rather than comments. Your naming is clear enough 👍 , so there's no need for comments here.

Same as below

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point! Agreed 🙂 I'll remove these comments now

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done! 🙂 2b5b320

Copy link
Copy Markdown
Contributor

@xiaogliu Vincent Liu (xiaogliu) left a comment

Choose a reason for hiding this comment

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

LGTM

@skyscanner-backpack-bot
Copy link
Copy Markdown

Visit https://backpack.github.io/storybook-prs/4158 to see this build running in a browser.

@xiaogliu Vincent Liu (xiaogliu) merged commit 6bc2bcd into main Jan 30, 2026
13 checks passed
@xiaogliu Vincent Liu (xiaogliu) deleted the feat/LUNA-3186-optional-marker-expand-phase branch January 30, 2026 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contribution minor Non breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants