feat(ui): one glossary-term picker, anchored on the add/edit icon - #33565
anuj-kumary wants to merge 42 commits into
Conversation
- Turn the dropdown chevron icon blue (text-fg-brand-primary) when there are active selections — applies to all FilterSelect trigger variants (button, input, chip) and FilterSelect.Tree triggers. - Remove outer tw:px-3 from TreeSelect popover so the footer divider spans full width. Tree list container uses pl-[14px] to align expand icons with the Select all checkbox. - Change TreeSelect inner content gap to gap-3 for near-perfect nested checkbox alignment with parent icon. - Use distinct Glossary/GlossaryTerm icons in the story data. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…TreeSelect Adds the pieces an icon-anchored picker needs, all opt-in so existing callers are unaffected: - `isOpen` / `onOpenChange` so a consumer can own the dropdown's open state - `commitMode="staged"` — toggles are buffered and reported once on Apply, reusing FilterSelect's Clear all / Cancel / Apply (N) footer, so a selection costs one request rather than one per checkbox - `renderTrigger` to render a consumer-owned trigger (an existing add/edit icon, say) in place of the built-in input/button Three fixes the above surfaced: - Portal the dropdown through react-aria's Popover rather than positioning it absolutely. The trigger routinely sits in a card or drawer that clips overflow, which cut the dropdown off. - Select-all / clear-all now write the local selection instead of relying on the parent echoing `value` back — which an uncontrolled consumer never does, so those actions silently did nothing there. - Escape closes the dropdown for every trigger variant, not just the input. Adds a Widget Edit Popover story showing the card + edit-icon + staged-Apply pattern the entity-page widgets use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Only the Cancel button restored the draft to `value`. Clicking away, pressing Escape, and the `close` callback handed to `renderTrigger` all just closed the popover, leaving the uncommitted draft in the selection. The trigger renders from that selection — the button variant's count badge, the input variant's chips, and `selectedCount` in `renderTrigger` — so a popover dismissed without Apply kept showing a selection the consumer had never been told about, until it was reopened and the resync effect ran. Routes all four paths through one `dismiss` helper that restores the draft when staged. Immediate mode is unaffected: it has no draft, and its single-select auto-close stays as it was. Reported by gitar-bot on #33489, which caught three of the four paths; the `renderTrigger` `close` callback had the same defect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A search returns matches nested under their parents, but every row rendered collapsed, so the hits were in the tree data and not in the DOM — typing a term name looked like it matched nothing but the top level. While a search is active the expanded set is derived from the results, so every branch is open; clearing the search returns to whatever the user had expanded manually. Reuses the parent-key walk that the selected-only view already had, hoisted out of its hook. The story's mock fetcher ignored `searchTerm` and could not show this, so it now mirrors the server (`getHierarchy: true`): a search returns each glossary with its matching terms already nested, grandchildren included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The search expansion was derived from the results, so it was recomputed on every render and a row the user collapsed sprang open again immediately. It is now applied as state, once per result set, with the pre-search expansion kept aside and restored when the search clears. Collapsing and re-expanding during a search behave normally, and clearing the search no longer leaves the search's branches open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keying the search expansion on `treeData` meant any update while the search was active — a lazy child load, a later response — reset the expanded set and reopened a row the user had deliberately collapsed. Each parent is now opened once, as it first appears, tracked in a ref. A new search term starts a fresh result set and opens its branches again. Reported by greptile on #33489. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The dropdown was pinned to `bottom left`, so react-aria kept it on screen by shifting it along the cross axis — detaching it from the trigger. Measure the trigger against the viewport instead: anchor to its left edge while the panel fits to the right, otherwise mirror to its right edge so it grows leftwards and stays attached. Re-measured on resize. Also moves the popover onto `Dropdown.Popover` for the shared raised surface and tightens the loading / empty-state rows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Glossary terms were picked through two unrelated surfaces — an inline Ant Design select that replaced the widget body, and a `selectable-list` popover in the Explore panel. Both are replaced by a single `GlossaryTermPicker` wrapping the core `TreeSelect`: the glossary hierarchy, lazy-loaded terms, server-side search, and a staged Apply that costs one PATCH per selection rather than one per checkbox. - `GlossaryTermTreeSelect` becomes `GlossaryTermPicker` + `useGlossaryTreeData`, splitting the data source out of the component. Its props are a `Pick` of `TreeSelectProps` plus the `TagLabel` in/out pair, so they cannot drift from the core component. - Glossaries are now checkable and carry an icon; ticking one selects its terms without ever being applied as a tag itself. A mutually exclusive glossary still gets no control, since exactly one of its terms may be chosen. - `TagsContainerV2` hangs the picker off the existing add/edit icon (header in the new look, inline icon in the legacy layouts) and keeps the body showing the terms instead of swapping it for a select. Classification tags are untouched. - `GlossaryTermsSection` — the Explore right panel, column detail panel and integration data-assets tab — moves to the same picker, which drops its mirrored editing state and the duplicated header markup. Playwright: the old selectors (`#tagsForm_tags`, `saveAssociatedTag`, `glossary-term-select-search-bar`, `.selectable-list-item`) no longer exist for glossary terms, so the flows move onto a shared `glossaryPicker` helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
anuj-kumary
added
UI
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
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 |
🔄 Playwright impact map auto-refreshedThis PR touched specs or UI source that changed the source→spec routing map. I regenerated What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit: python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit # or a separate commit |
This PR only consumes TreeSelect; the component itself is #33489's to change. The dropdown width fix it carried will follow separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comments Outside DiffThese findings sit on lines the diff does not cover, so they could not be posted inline. Each one leaves this list once its file changes.
|
Jest test CoverageUI tests summary
|
🔄 Playwright impact map auto-refreshedThis PR touched specs or UI source that changed the source→spec routing map. I regenerated What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit: python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit # or a separate commit |
…etadata/OpenMetadata into feat/glossary-term-picker
🔄 Playwright impact map auto-refreshedThis PR touched specs or UI source that changed the source→spec routing map. I regenerated What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit: python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit # or a separate commit |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
…etadata/OpenMetadata into feat/glossary-term-picker
🚦 Removed from the merge queue —
|
Conflict: playwright/eslint-rules/tests/corpus.test.mjs — the suppressions ratchet. Resolved to the counts the merged eslint-suppressions.json actually produces: main's new no-blanket-test-slow key and its lower justified-rule-disable/no-skipped-test totals, this branch's burned-down no-positional-locator (1224) and no-wait-for-selector (25). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🚦 Removed from the merge queue —
|
Conflict: playwright/eslint-rules/tests/corpus.test.mjs — the suppressions ratchet. Both sides burned down no-positional-locator from different files, so the merged eslint-suppressions.json totals 1202, below either side's number (1224 here, 1217 on main). Resolved to the merged file's actual count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🔄 Playwright impact map auto-refreshedThis PR touched specs or UI source that changed the source→spec routing map. I regenerated What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit: python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit # or a separate commit |
Code Review ✅ Approved 1 closed / 1 findings🟡 Medium risk Consolidates glossary-term selection onto a shared ✅ 1 closed✅ Bug: Exclusivity bypassed when seeded sibling is off the search tree
OptionsDisplay: compact → Counting what did not apply, without listing it. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
| Count | Rule |
|---|---|
| 13 | react-hooks/exhaustive-deps |
| 2 | openmetadata-imports/review-sequential-api-calls |
| 1 | openmetadata-imports/no-internal-barrel-imports |
All findings
| Location | Rule | Message | |
|---|---|---|---|
| 🟡 | src/components/Domain/AddDomainForm/AddDomainForm.component.tsx:90:1 |
openmetadata-imports/no-internal-barrel-imports |
Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph. |
| 🟡 | src/components/Tag/TagsContainerV2/TagsContainerV2.tsx:100:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has a missing dependency: 'columnData?.name'. Either include it or remove the dependency array. |
| 🟡 | src/components/Tag/TagsContainerV2/TagsContainerV2.tsx:127:5 |
react-hooks/exhaustive-deps |
React Hook useCallback has a missing dependency: 'updateActiveTagDropdownKey'. Either include it or remove the dependency array. |
| 🟡 | src/components/Tag/TagsContainerV2/TagsContainerV2.tsx:148:5 |
react-hooks/exhaustive-deps |
React Hook useMemo has an unnecessary dependency: 'tags.tagType'. Either exclude it or remove the dependency array. |
| 🟡 | src/components/Tag/TagsContainerV2/TagsContainerV2.tsx:164:5 |
react-hooks/exhaustive-deps |
React Hook useMemo has missing dependencies: 'tagType' and 'tags'. Either include them or remove the dependency array. |
| 🟡 | src/components/Tag/TagsContainerV2/TagsContainerV2.tsx:307:5 |
react-hooks/exhaustive-deps |
React Hook useMemo has missing dependencies: 'sizeCap', 'tagType', and 'tags'. Either include them or remove the dependency array. |
| 🟡 | src/components/Tag/TagsContainerV2/TagsContainerV2.tsx:329:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has a missing dependency: 'tagType'. Either include it or remove the dependency array. Outer scope values like 'getTagPlaceholder' aren't val |
| 🟡 | src/components/Tag/TagsContainerV2/TagsContainerV2.tsx:363:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has missing dependencies: 't', 'tagType', and 'tags'. Either include them or remove the dependency array. |
| 🟡 | src/components/Tag/TagsContainerV2/TagsContainerV2.tsx:377:5 |
react-hooks/exhaustive-deps |
React Hook useMemo has a missing dependency: 't'. Either include it or remove the dependency array. |
| 🟡 | src/components/Tag/TagsContainerV2/TagsContainerV2.tsx:410:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has missing dependencies: 'handleAddClick' and 't'. Either include them or remove the dependency array. |
| 🟡 | src/components/Tag/TagsContainerV2/TagsContainerV2.tsx:439:5 |
react-hooks/exhaustive-deps |
React Hook useMemo has a missing dependency: 't'. Either include it or remove the dependency array. |
| 🟡 | src/components/Tag/TagsContainerV2/TagsContainerV2.tsx:458:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has missing dependencies: 'sizeCap', 'tagType', and 'tags'. Either include them or remove the dependency array. |
| 🟡 | src/components/Tag/TagsContainerV2/TagsContainerV2.tsx:500:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has a missing dependency: 'newLook'. Either include it or remove the dependency array. |
| 🟡 | src/components/Tag/TagsContainerV2/TagsContainerV2.tsx:538:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has missing dependencies: 'columnData?.fqn' and 'entityFqn'. Either include them or remove the dependency array. |
| 🟡 | src/components/common/GlossaryTermPicker/useGlossaryTreeData.tsx:92:27 |
openmetadata-imports/review-sequential-api-calls |
Review these sequential API requests. If they are independent, start them together with Promise.all/Promise.allSettled; keep sequencing only when data-dependent |
| 🟡 | src/components/common/GlossaryTermPicker/useGlossaryTreeData.tsx:109:38 |
openmetadata-imports/review-sequential-api-calls |
Review these sequential API requests. If they are independent, start them together with Promise.all/Promise.allSettled; keep sequencing only when data-dependent |
Fix locally (fast - only checks files changed in this branch):
make ui-checkstyle-changed
|
3 participants
Summary
Glossary terms were picked through two unrelated surfaces: an inline Ant Design select that replaced the widget body, and a
selectable-listpopover in the Explore panel. Both now use oneGlossaryTermPickerwrapping the coreTreeSelect— glossary hierarchy, lazy-loaded terms, server-side search, and a staged Apply that costs one PATCH per selection instead of one per checkbox.Changes
GlossaryTermTreeSelect→GlossaryTermPicker+useGlossaryTreeData, splitting the data source out of the component. Props are aPickofTreeSelectPropsplus theTagLabelin/out pair, so they cannot drift from the core component.data). A mutually exclusive glossary still gets no control, since exactly one of its terms may be chosen.TagsContainerV2hangs the picker off the existing add/edit icon — header in the new look, inline icon in the legacy layouts, exactly one anchor per layout — and keeps the body showing the terms instead of swapping it for a select. Classification tags are untouched.GlossaryTermsSection(Explore right panel, column detail panel, integration data-assets tab) moves to the same picker, dropping its mirrored editing state and duplicated header markup.TreeSelect: the dropdown now mirrors to the trigger's right edge when there is no room on the right, instead of letting react-aria shift it off its anchor; and the field-width variant measures the trigger, because--trigger-widthis only published by react-aria's own trigger components, never by aPopovergiven a baretriggerRef— sow-(--trigger-width)resolved to nothing and the panel shrink-to-fit.Playwright
The old selectors (
#tagsForm_tags,saveAssociatedTag,glossary-term-select-search-bar,.selectable-list-item) no longer exist for glossary terms. All glossary flows move onto a sharedplaywright/utils/glossaryPicker.ts.eslint-suppressions.jsondrops one staleno-positional-locatorentry (3 → 2).Testing
GlossaryTermPicker,GlossaryTermsSection,TagsContainerV2,AddDomainForm,DataAssetSummaryPanelV1).eslintclean on every touched file;tsc --noEmitclean forui-core-componentsand no new errors in the Playwright project.Downstream
Collate's Automator form renders
TagsContainerV2withtagType={TagSource.Glossary}, so its Playwright helpers need the matching update — open-metadata/openmetadata-collate PR to follow, pointed at this branch to verify the pair before either merges.🤖 Generated with Claude Code
The implementation appears behaviorally safe, but the outstanding component-library translation requirement must be satisfied before merging.
Findings
Summary
This PR consolidates glossary-term selection around a shared staged
GlossaryTermPickerand extends the coreTreeSelectto support consumer-owned triggers, controlled open state, staged commits, lazy hierarchy loading, and anchored dropdown placement.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Add/Edit icon] --> B[GlossaryTermPicker] B --> C[TreeSelect staged draft] C --> D[Lazy glossary hierarchy] C --> E[Server-side search] C --> F{User action} F -->|Apply| G[Single onChange / PATCH] F -->|Cancel, Escape, outside click, trigger close| H[Discard draft]Reviews (15) · Last reviewed commit: "chore(playwright): auto-refresh impact-m..."