[WEB-4748] chore: placement helper fn added and code refactor#7627
[WEB-4748] chore: placement helper fn added and code refactor#7627sriramveeraghanta merged 2 commits intopreviewfrom
Conversation
WalkthroughCentralizes placement types and conversion into a new shared utility (utils/placement.ts). Popover and Tooltip components remove local types/mappings and now import TPlacement/TSide/TAlign and convertPlacementToSideAndAlign. Public prop types are updated to reference shared types; local exports for placement types/functions are removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Component as Popover/Tooltip
participant Utility as utils/placement.convertPlacementToSideAndAlign
User->>Component: render({ placement? / position?, side?, align? })
alt placement/position provided
Component->>Utility: convertPlacementToSideAndAlign(placement)
Utility-->>Component: { side, align }
Component->>Component: use returned side/align
else explicit side/align provided
Component->>Component: use provided side/align
end
Component-->>User: UI rendered with computed side/align
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
anmolsinghbhatia
requested review from
prateekshourya29 and
vamsikrishnamathala
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/propel/src/tooltip/root.tsx (1)
26-33: Side/align are effectively ignored due toposition = "top"default; fix precedence detection.Because
positionhas a default,if (position)is always true, soside/aligncan never take effect. This silently changes behavior for callers who only setside/align.Use prop-presence detection to prefer:
- position when explicitly provided, OR when neither side nor align is provided (preserve “top” default),
- otherwise use side/align.
const { tooltipHeading, tooltipContent, - position = "top", + position = "top", children, disabled = false, className = "", openDelay = 200, - side = "bottom", - align = "center", + side = "bottom", + align = "center", sideOffset = 10, closeDelay, isMobile = false, } = props; - const { finalSide, finalAlign } = React.useMemo(() => { - if (position) { - const converted = convertPlacementToSideAndAlign(position); - return { finalSide: converted.side, finalAlign: converted.align }; - } - return { finalSide: side, finalAlign: align }; - }, [position, side, align]); + // Determine precedence: use `position` if explicitly provided, or when neither `side` nor `align` are provided. + const hasPositionProp = Object.prototype.hasOwnProperty.call(props, "position"); + const hasSideProp = Object.prototype.hasOwnProperty.call(props, "side"); + const hasAlignProp = Object.prototype.hasOwnProperty.call(props, "align"); + const shouldUsePosition = hasPositionProp || (!hasSideProp && !hasAlignProp); + + const { finalSide, finalAlign } = React.useMemo(() => { + if (shouldUsePosition) { + const converted = convertPlacementToSideAndAlign(position); + return { finalSide: converted.side, finalAlign: converted.align }; + } + return { finalSide: side, finalAlign: align }; + }, [position, side, align, shouldUsePosition]);Also applies to: 37-43
🧹 Nitpick comments (6)
packages/propel/src/utils/placement.ts (3)
22-39: Prefer an exhaustive typed Record over Map for compile-time coverage.Using a
Record<TPlacement, {side; align}>withsatisfiesguarantees every union member is mapped and removes the need for a runtime fallback. It’s also simpler and more treeshake-friendly thanMap.Apply:
-// placement conversion map -const PLACEMENT_MAP = new Map<TPlacement, { side: TSide; align: TAlign }>([ - ["auto", { side: "bottom", align: "center" }], - ["auto-start", { side: "bottom", align: "start" }], - ["auto-end", { side: "bottom", align: "end" }], - ["top", { side: "top", align: "center" }], - ["bottom", { side: "bottom", align: "center" }], - ["left", { side: "left", align: "center" }], - ["right", { side: "right", align: "center" }], - ["top-start", { side: "top", align: "start" }], - ["top-end", { side: "top", align: "end" }], - ["bottom-start", { side: "bottom", align: "start" }], - ["bottom-end", { side: "bottom", align: "end" }], - ["left-start", { side: "left", align: "start" }], - ["left-end", { side: "left", align: "end" }], - ["right-start", { side: "right", align: "start" }], - ["right-end", { side: "right", align: "end" }], -]); +// placement conversion map (exhaustive over TPlacement) +const PLACEMENT_MAP = { + "auto": { side: "bottom", align: "center" }, + "auto-start": { side: "bottom", align: "start" }, + "auto-end": { side: "bottom", align: "end" }, + "top": { side: "top", align: "center" }, + "bottom": { side: "bottom", align: "center" }, + "left": { side: "left", align: "center" }, + "right": { side: "right", align: "center" }, + "top-start": { side: "top", align: "start" }, + "top-end": { side: "top", align: "end" }, + "bottom-start": { side: "bottom", align: "start" }, + "bottom-end": { side: "bottom", align: "end" }, + "left-start": { side: "left", align: "start" }, + "left-end": { side: "left", align: "end" }, + "right-start": { side: "right", align: "start" }, + "right-end": { side: "right", align: "end" }, +} satisfies Record<TPlacement, { side: TSide; align: TAlign }>;
41-47: Remove unreachable fallback and use object index.With the exhaustive
Recordapproach, a fallback should never be needed; returning a default can mask missing mappings during future edits.-export function convertPlacementToSideAndAlign(placement: TPlacement): { - side: TSide; - align: TAlign; -} { - return PLACEMENT_MAP.get(placement) || { side: "bottom", align: "center" }; -} +export function convertPlacementToSideAndAlign(placement: TPlacement): { + side: TSide; + align: TAlign; +} { + return PLACEMENT_MAP[placement]; +}
2-20: Nit: Type names without “T” prefix are more idiomatic in TS.
Placement,Side,Alignread cleaner and align with common style, but this is subjective. If you prefer theTprefix across the codebase, keep it consistent.packages/propel/src/tooltip/root.tsx (2)
6-20: Consider aligning prop naming:positionvsplacement.Popover uses
placementwhile Tooltip usesposition. If feasible, consider aliasingplacementin Tooltip props (with deprecation notice) for consistency.
26-33: Document precedence in props JSDoc.A short note that
positiontakes precedence overside/align(and defaults to"top"when neither is provided) will prevent confusion.I can add JSDoc to
ITooltipPropsif you want.packages/propel/src/popover/root.tsx (1)
33-41: Minor: Consider documenting precedence in the component props.A brief comment in
PopoverContentPropsor component JSDoc aboutplacementtaking precedence overside/alignwould help usage clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/propel/src/popover/root.tsx(1 hunks)packages/propel/src/tooltip/root.tsx(1 hunks)packages/propel/src/utils/placement.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/propel/src/tooltip/root.tsx (1)
packages/propel/src/utils/placement.ts (3)
TPlacement(2-17)TSide(19-19)TAlign(20-20)
packages/propel/src/popover/root.tsx (1)
packages/propel/src/utils/placement.ts (3)
TPlacement(2-17)TAlign(20-20)TSide(19-19)
🔇 Additional comments (7)
packages/propel/src/utils/placement.ts (1)
24-26: Confirm intent for “auto” placements mapping to bottom variants*I attempted to locate any occurrences of
placement="auto",placement="auto-start", orplacement="auto-end"across our.tsand.tsxfiles and found none. It’s possible these variants aren’t used or may only appear in.js/.jsxexamples or documentation. Since normalizing all “auto*” cases to bottom variants removes any viewport-aware flipping (a breaking change if consumers expected dynamic flips), please:
- Manually verify that there are no existing uses of
"auto","auto-start", or"auto-end"in any.js/.jsxfiles, examples, or docs.- If the behavior change is intentional, update our documentation to note that
auto*now maps to bottom-only placements (no flipping).- Otherwise, consider preserving the original “auto” semantics by routing dynamic flipping through the underlying Positioner’s collision/flip props rather than normalizing here.
packages/propel/src/tooltip/root.tsx (3)
4-4: Good move centralizing placement types/logic.Importing shared
TPlacement/TSide/TAlignand the converter reduces duplication and keeps Tooltip aligned with Popover.
50-67: LGTM on Positioner usage.
side,align, andsideOffsetflow correctly intoBaseTooltip.Positioner, andclassNamemerging is clean.
6-20: Verify removal of legacy placement exportsPlease manually confirm that no consumer code is importing the now-removed placement-related types or helpers from the Tooltip package. Specifically, search for any references to the following identifiers in your application or other packages:
TPositionPlacementSideAlignconvertPlacementToSideAndAlignIf any usages are found, they should be updated to use the new props on
ITooltipProps(position?: TPlacement,side?: TSide,align?: TAlign) or those legacy types/functions should be re-exported where needed.packages/propel/src/popover/root.tsx (3)
3-3: Nice consolidation of placement utilities.Importing shared types and converter removes duplication and keeps Popover/Tooltip behavior in sync.
25-31: Precedence logic reads correctly.Using
placementwhen provided, otherwise falling back toside/alignmatches expected behavior and pairs well with the shared converter.
6-11: No downstream breakage detected from type name changes
A repository-wide search found no imports of the oldPlacement,Side,Align, orconvertPlacementToSideAndAlignexports.
- The only import from
packages/propel/src/popover/rootisPopoverinpackages/propel/src/combobox/combobox.tsx, which isn’t affected by the type renames.
|
@anmolsinghbhatia Format check is failing. Can you look into it? |
…l-placement-helper-fn
4 participants
Description
This PR adds a placement helper utility function to the Propel package.
Type of Change
Summary by CodeRabbit
New Features
Refactor