Skip to content

refactor(core-components): forward popover props from FieldDocPopover#30165

Open
ShaileshParmar11 wants to merge 2 commits into
open-metadata:mainfrom
ShaileshParmar11:refactor/field-doc-popover-spread
Open

refactor(core-components): forward popover props from FieldDocPopover#30165
ShaileshParmar11 wants to merge 2 commits into
open-metadata:mainfrom
ShaileshParmar11:refactor/field-doc-popover-spread

Conversation

@ShaileshParmar11

@ShaileshParmar11 ShaileshParmar11 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #30143 (review comment) — @siddhant1 asked that FieldDocPopover spread its props onto the underlying Popover rather than threading them one by one, and that className be extracted and merged rather than dropped.

  • FieldDocPopoverProps now extends the underlying PopoverProps, and the rest is spread onto <Popover>, so future popover props don't need threading individually.
  • className is accepted and merged onto the card's own styling via cx (twMerge), matching the existing pattern in table.tsx — and handling react-aria's render-prop form (string | (renderProps) => string).
  • Only the props this component genuinely owns are Omitted: isOpen / isNonModal (its open behaviour) and triggerRef (the field it anchors to). triggerRef is applied after the spread so the internal anchor can't be overridden.

Why offset / maxHeight stay outside the spread

They're destructured with their defaults rather than riding the spread. HookForm forwards them unconditionally (maxHeight={fieldDocMaxHeight}), so when a consumer doesn't set them the key still exists with value undefined — and a spread with an existing key wins:

{ maxHeight: 480, ...{ maxHeight: undefined } }  // -> { maxHeight: undefined }

That would silently drop the height cap and let the hint card grow to the full page height again (the bug #30143 fixed). A default parameter resolves undefined back to the default, so it's immune to how callers forward it.

Behaviour is unchanged:

Consumer passes Result
maxHeight={undefined} (HookForm today) 480 — default kept
maxHeight={300} 300 — override respected
nothing 480 / offset 16

Testing

Prettier, ESLint and tsc --noEmit all clean. No behavioural change intended — pure refactor.

🤖 Generated with Claude Code

Greptile Summary

This PR forwards supported popover options through FieldDocPopover. The main changes are:

  • Extends FieldDocPopoverProps from React Aria's PopoverProps.
  • Merges caller-provided classes with the card's default styles.
  • Preserves internal control of open state and the anchor reference.
  • Keeps explicit defaults for offset and maximum height.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.

Important Files Changed

Filename Overview
openmetadata-ui-core-components/src/main/resources/ui/src/components/application/form-field/field-doc-popover.tsx Forwards supported React Aria popover props while protecting component-owned behavior and merging custom classes.

Reviews (4): Last reviewed commit: "Merge branch 'main' into refactor/field-..." | Re-trigger Greptile

Context used (3)

  • Context used - CLAUDE.md (source)
  • Context used - openmetadata-ui-core-components/CLAUDE.md (source)
  • Context used - AGENTS.md (source)

Copilot AI review requested due to automatic review settings July 17, 2026 07:16

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown
Contributor

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

  • No GitHub issue is linked. Link an issue in the Development section of the PR (or add Fixes #12345 to the description). For a same-org cross-repo issue, add Fixes open-metadata/<repo>#123 to the description.

The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically.

Maintainers can bypass this check by adding the skip-pr-checks label.

@github-actions github-actions Bot added safe to test Add this label to run secure Github workflows on PRs UI UI specific issues labels Jul 17, 2026
@ShaileshParmar11
ShaileshParmar11 force-pushed the refactor/field-doc-popover-spread branch from 95fe557 to ba92bd9 Compare July 17, 2026 07:23
Copilot AI review requested due to automatic review settings July 17, 2026 07:23

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Extend FieldDocPopoverProps from the underlying PopoverProps and spread the
rest onto Popover, so future popover props no longer need threading one by
one. className is accepted as a string and merged onto the card's own styling
via cx (twMerge), so callers can extend it without replacing it.

offset/maxHeight stay destructured with their defaults: consumers (e.g.
HookForm) forward them unconditionally, so an undefined value would otherwise
land in the spread and clobber the default. triggerRef is applied after the
spread so the internal anchor can't be overridden.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@ShaileshParmar11
ShaileshParmar11 force-pushed the refactor/field-doc-popover-spread branch from ba92bd9 to c4016d2 Compare July 17, 2026 07:29
Copilot AI review requested due to automatic review settings July 17, 2026 07:29

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@ShaileshParmar11
ShaileshParmar11 enabled auto-merge (squash) July 17, 2026 08:10
@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

🟡 Playwright Results — all passed (33 flaky)

✅ 4538 passed · ❌ 0 failed · 🟡 33 flaky · ⏭️ 95 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 435 0 5 16
✅ Shard 2 11 0 0 0
🟡 Shard 3 817 0 15 8
🟡 Shard 4 822 0 1 18
🟡 Shard 5 839 0 2 5
🟡 Shard 6 787 0 1 46
🟡 Shard 7 827 0 9 2
🟡 33 flaky test(s) (passed on retry)
  • Flow/TestConnectionModal.spec.ts › Edit Connection click dismisses the modal (shard 1, 1 retry)
  • Pages/Lineage/LineageRightPanel.spec.ts › Verify custom properties tab IS visible for supported type: metric (shard 1, 1 retry)
  • Pages/SearchSettings.spec.ts › Preview config reflects reverted n-gram weight after save (shard 1, 1 retry)
  • Pages/SearchSettings.spec.ts › Latest preview config wins when a superseded request resolves late (shard 1, 1 retry)
  • Flow/SearchRBAC.spec.ts › a fully denied user sees neither asset type when browsing (shard 1, 1 retry)
  • Features/BulkEditEntity.spec.ts › Database (shard 3, 2 retries)
  • Features/BulkEditEntity.spec.ts › Table (shard 3, 1 retry)
  • Features/ColumnBulkOperations.spec.ts › should clear individual filter and update URL (shard 3, 1 retry)
  • Features/ColumnBulkOperations.spec.ts › should show Service filter chip from URL (shard 3, 1 retry)
  • Features/ContextCenterArchive.spec.ts › archive page lazy-loads more rows on scroll within its own scroll container (shard 3, 1 retry)
  • Features/ContextCenterArticles.spec.ts › Article listing search filters, clears, and shows empty state (shard 3, 2 retries)
  • Features/ContextCenterArticles.spec.ts › Article list cards, recently viewed widget, and pagination work (shard 3, 1 retry)
  • Features/ContextCenterArticles.spec.ts › Text formatting (shard 3, 1 retry)
  • Features/ContextCenterArticles.spec.ts › description: switching articles does not bleed unsaved content into next article (shard 3, 1 retry)
  • Features/ContextCenterMemories.spec.ts › clearing search restores the unfiltered list (shard 3, 1 retry)
  • Features/ContextCenterMemories.spec.ts › adding a linked asset in edit mode shows entity badge on the row (shard 3, 1 retry)
  • Features/ContextCenterPermission.spec.ts › user with view-only permission cannot see restore or delete actions on an archived document (shard 3, 1 retry)
  • Features/DataQuality/TestCaseImportExportBasic.spec.ts › should show validation errors for invalid CSV (shard 3, 1 retry)
  • Features/Glossary/GlossaryAdvancedOperations.spec.ts › should change domain on glossary (shard 3, 1 retry)
  • Features/Glossary/GlossaryHierarchy.spec.ts › should move term with children to different glossary (shard 3, 1 retry)
  • Features/SearchExport.spec.ts › Export queues a background job and downloads from the jobs tray (shard 4, 1 retry)
  • Flow/PersonaFlow.spec.ts › Set default persona for team should work properly (shard 5, 1 retry)
  • Pages/CustomProperties.spec.ts › Should search custom properties for apiCollection in right panel (shard 5, 1 retry)
  • Pages/ExplorePageRightPanel_KnowledgeCenter.spec.ts › Should remove user owner for knowledgeCenter (shard 6, 1 retry)
  • Pages/GlossaryImportExport.spec.ts › Glossary Bulk Import Export (shard 7, 1 retry)
  • Pages/GlossaryImportExport.spec.ts › Import partial success - some terms pass, some fail (shard 7, 1 retry)
  • Pages/Lineage/LineageFilters.spec.ts › Verify Impact Analysis service filter selection (shard 7, 1 retry)
  • Pages/Lineage/LineageRightPanel.spec.ts › Verify custom properties tab is NOT visible for pipelineService in platform lineage (shard 7, 1 retry)
  • Pages/Lineage/LineageRightPanel.spec.ts › Verify custom properties tab is NOT visible for apiService in platform lineage (shard 7, 1 retry)
  • Pages/Tag.spec.ts › Verify Tag UI (shard 7, 1 retry)
  • ... and 3 more

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

Copilot AI review requested due to automatic review settings July 18, 2026 04:42

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@gitar-bot

gitar-bot Bot commented Jul 18, 2026

Copy link
Copy Markdown
Code Review ✅ Approved

Refactors FieldDocPopover to forward underlying popover props via spread and properly merges className. No issues found.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants