Conversation
…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.
valkirilov
previously approved these changes
Nov 28, 2025
…styled-components # Conflicts: # redisinsight/ui/src/components/base/popover/RiPopover.tsx # redisinsight/ui/src/components/base/popover/types.ts
pd-redis
requested review from
ArtemHoruzhenko,
DimoHG,
KrumTy,
dantovska and
pawelangelow
as code owners
Contributor
Code Coverage - Frontend unit tests
Test suite run success5568 tests passing in 708 suites. Report generated by 🧪jest coverage report action from 3abf5bd |
Comment on lines
+31
to
+42
| if (button !== undefined && trigger !== undefined) { | ||
| console.warn( | ||
| "[RiPopover]: Both 'button' and 'trigger' props are provided. Using 'trigger'. Please migrate to 'trigger' prop.", | ||
| ) | ||
| } | ||
|
|
||
| // Warn if both panelClassName and className are provided | ||
| if (panelClassName !== undefined && className !== undefined) { | ||
| console.warn( | ||
| "[RiPopover]: Both 'panelClassName' and 'className' props are provided. Using 'className'. Please migrate to 'className' prop.", | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
I guess we want to log these on dev only - process.env.NODE_ENV !== 'production'.
And additionally if we wrap them in useEffect({}, []) to display warnings once
Contributor
Author
There was a problem hiding this comment.
We don't do any of that for any component and if accepted, I would prefer to leave these in to be loud and obnoxious, so that we don't leave them as is
Contributor
There was a problem hiding this comment.
I don't have a strong opinion on it. It seems strange to have such logs for the production build to me.
Contributor
|
Suggestion for the PR name: |
valkirilov
approved these changes
Jan 30, 2026
pawelangelow
approved these changes
Jan 30, 2026
pd-redis
deleted the
fe/bugfix/refactor-ri-tooltip-to-better-use-styled-components
branch
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
3 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.
What
[Tech Debt]
Refactored
RiPopovercomponent to improve styled-components compatibility and extensibility while maintaining full backwards compatibility.Changes
triggerprop (optional): New preferred prop for popover trigger element, works alongside existingbuttonprop.aliasof button, withthe idea to move to it entirely over time and remove
buttonusagesclassNameprop (optional): New preferred prop for popover content wrapper styling, using standard prop name for better styled-components integration.standaloneprop (optional): Whentrue, 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.buttonandpanelClassNameprops continue to work unchanged:buttonalways wraps in span by default (legacy behavior preserved)panelClassNamefalls back whenclassNameis not providedTech Decisions
standaloneprop: Rather than automatically wrapping trigger in span,use an explicit
standaloneprop to control trigger wrapping.This gives developers clear if they need it.
classNameinstead ofpanelClassNamealigns 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 contentsWhy
This refactoring enables better integration with styled-components and similar styling libraries:
classNameprop, making it easy to style RiPopover's component with styled-components.inlineelement, other timesblock,flexor whatever element for trigger of the popover. This is the Element that the user clicks on to show the popover.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).
spanis used, don't need to change anything.span,divorstyled-componentand you are done.Testing
RiPopoverremain unaffectedManual Testing
RiPopoverusages continue to work (e.g.,CancelButton,EditablePopover,ConfirmationPopover,TagsCellHeader)triggerorbuttonprop withstandalone={true}(renders directly without span wrapper)triggerorbuttonprop withoutstandalone(wraps in span as expected)classNameprop works correctly for styled-components integrationbutton/triggerorpanelClassName/classNameare providedNote
Refactors
RiPopoverto improve flexibility and styled-components compatibility while preserving backwards compatibility.trigger,className, andstandaloneprops; preferstriggerover legacybuttonandclassNameoverpanelClassName, with console warnings on conflictsstandaloneis true; wraps scalars forstandaloneRiPopover.constantsand renames toANCHOR_POSITION_MAPandPANEL_PADDING_SIZE_MAP; updates types toRiPopover.typesand re-exports viaindex.tspanelPaddingSizeusingPANEL_PADDING_SIZE_MAP; preserves existing behavior foronClickOutside, focus, placement, and alignRiPopover.spec.tsx) covering rendering, new/legacy props, conflict warnings, padding, and scalar triggersWritten by Cursor Bugbot for commit 3abf5bd. This will update automatically on new commits. Configure here.