Skip to content

Conversation

@pd-redis
Copy link
Collaborator

What

[Tech Debt]
Refactored RiPopover component to improve styled-components compatibility and extensibility while maintaining full backwards compatibility.

Changes

  • Added trigger prop (optional): New preferred prop for popover trigger element, works alongside existing button prop.
    • the idea is, that trigger sounds more close to what that prop is doing, right now you can consider this alias of button, with
      the idea to move to it entirely over time and remove button usages
  • Added className prop (optional): New preferred prop for popover content wrapper styling, using standard prop name for better styled-components integration.
    • it is passed to Popover.Compose, so it RiPopover can be styled directly and that will be reflected on the content wrapper
  • Added standalone prop (optional): When true, the trigger renders directly without a span wrapper to avoid DOM interference with styling/layout. Requires the trigger element to be a base DOM element (div, span, etc.) or a component that forwards refs.
  • Backwards compatibility: Existing button and panelClassName props continue to work unchanged:
    • button always wraps in span by default (legacy behavior preserved)
    • panelClassName falls back when className is not provided
  • Migration warnings: Console warnings when both old and new props are provided together

Tech Decisions

  • Backwards compatible approach: Instead of breaking changes, added new optional props that work alongside existing ones. This allows gradual migration without disrupting existing code.
  • Controwll wrapping with standalone prop: Rather than automatically wrapping trigger in span,
    use an explicit standalone prop to control trigger wrapping.
    This gives developers clear if they need it.
  • Standard prop names: Using className instead of panelClassName aligns with styled-components and other CSS-in-JS libraries that expect standard React prop names. And will work out of the box for custom styling of popover contents

Why

This refactoring enables better integration with styled-components and similar styling libraries:

  1. Standard prop names: Styled-components work seamlessly with standard className prop, making it easy to style RiPopover's component with styled-components.
  2. Use the best component for the usecase:
  • sometimes you need inline element, other times block, flex or whatever element for trigger of the popover. This is the Element that the user clicks on to show the popover.
  • Right now to achieve this, you need an anchorClassName (found more than 20 usages in the codebase).
    It should be a css class stored somewhere (global or module stylesheet), which while it is not a problem in
    general, creates discrepancy with how we want to style things (using styled-components).
  • If you don't care that defalut wrapper span is used, don't need to change anything.
  • If you however need custom wrapper, just wrap your trigger with a basic span, div or styled-component and you are done.

Testing

  • No breaking changes - all ~40 files using RiPopover remain unaffected

Manual Testing

  • Verified existing RiPopover usages continue to work (e.g., CancelButton, EditablePopover, ConfirmationPopover, TagsCellHeader)
  • Tested new trigger or button prop with standalone={true} (renders directly without span wrapper)
  • Tested new trigger or button prop without standalone (wraps in span as expected)
  • Tested className prop works correctly for styled-components integration
  • Verified warnings appear in console when both button/trigger or panelClassName/className are provided

…better extensibility

Refactor RiPopover to add optional trigger and className props while maintaining backwards compatibility. React elements in trigger render directly without span wrapper for styled-components support.
…better extensibility

Refactor RiPopover to replace `button` with `trigger` and className props while maintaining backwards compatibility.
simplify RiPopover trigger rendering with standalone prop, so the trigger can be any custom element that can receive `ref` and pass to it DOM element
… handling

Add test suite for RiPopover covering all props and behaviors. Fix standalone prop to wrap scalar values in span for asChild compatibility. Improve code readability.
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