Skip to content

fix: preserve ordinary popover margins by default - #8

Merged
rdlabo merged 1 commit into
mainfrom
fix/projected-popover-alignment-only
Oct 1, 2026
Merged

rdlabo merged 1 commit into
mainfrom
fix/projected-popover-alignment-only

Conversation

@rdlabo

@rdlabo rdlabo commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Keep the existing 8px vertical adjustment for ordinary popovers. The enter animation now accepts an explicit preserveHorizontalAlignment option, used by the theme only for projected Vertical Bars controls. Horizontal alignment and viewport clamping are opt-in. Existing tests cover both default margins and opt-in alignment on all four horizontal sides; all 20 tests and the TypeScript build pass.


Devin Review

@devin-ai-integration devin-ai-integration Bot left a comment

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.

Devin Review found 2 potential issues.

Devin Review

Comment thread src/popover/utils.ts
Comment on lines +621 to 623
const horizontal = preserveHorizontalAlignment && (side === 'left' || side === 'right' || side === 'start' || side === 'end');
let top = !isReplace ? coordTop + (horizontal ? 0 : POPOVER_IOS_BODY_MARGIN) : coordTop - triggerHeight;
if (horizontal && !isReplace) top = Math.max(bodyPadding, Math.min(top, bodyHeight - bodyPadding - contentHeight));

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.

🟡 通常の横配置ポップオーバーが画面外に切れる

横配置でオプションを指定しないと、8px の余白だけが加わり、従来の上下方向のクランプが働きません。画面端付近ではポップオーバーの内容が画面外に出ます。

Learn more

この関数はポップオーバーの座標を画面内に調整します。横配置では従来、bodyPadding を使って上下端を制限していました。今回、余白の有無とクランプの有無を同じ horizontal 条件で切り替えたため、通常の横配置でクランプまで失われます。トリガーが下端付近にあると、コンテンツの下部が見えなくなります。

Example: 画面高 636px、コンテンツ高 52px、coordTop が 620px、side が right、bodyPadding が 5px の場合、デフォルトの top は 628px になります。従来は 579px に制限され、コンテンツ全体が表示されました。

Recommended fix: 横配置かどうかの判定と、余白を省くためのオプトイン判定を分離してください。通常配置には 8px の余白を残しつつ、余白を適用した後の座標も上下の画面内に制限し、オプトイン時の配置も維持してください。

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

results.referenceCoordinates,
referenceSizeEl?.getBoundingClientRect(),
isReplace,
opts.preserveHorizontalAlignment === true,

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.

🔍 Vertical Bars のオプトイン指定箇所を確認

説明にあるテーマ側の preserveHorizontalAlignment: true 指定は、この変更には含まれていません。投影コントロールの配置を維持するため、対応するテーマ側の変更が適用されるか確認してください。

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

@rdlabo
rdlabo merged commit 5663c0b into main Oct 1, 2026
6 of 8 checks passed
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown

npm beta published

CI passed for the merge commit 5663c0b4afcd. Install the immutable version with:

npm install @rdlabo/ionic-theme-utils@0.1.3-beta.pr8.sha5663c0b4afcd

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.

1 participant