FE: generalize ClassSetConfig and DistributionConfigDialog for categorical values - #1736
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
❌ Fast Track: checks did not passFailed guardrails: frontend/complexity (Guardrail threw: Cannot find module 'eslint'
Reflects |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ea245b987
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }: Props = $props(); | ||
|
|
||
| const maxN = $derived(allClasses.length); | ||
| const maxN = $derived(items?.length ?? allClasses.length); |
There was a problem hiding this comment.
When items is supplied only to give the manual selector stable values/labels, this changes the top-N cap from the full allClasses list to items.length. If those lists differ, such as a manual list that omits a non-selectable semantic bucket while the chart still has it in allClasses, the input max and the “All” action cap the chart too low so users cannot show every category. Keep the top-N bound tied to the full chart data, or make items a documented complete replacement for allClasses.
Useful? React with 👍 / 👎.
| {allClasses} | ||
| {items} | ||
| {itemNounPlural} | ||
| itemNoun={itemNounPlural === 'classes' ? 'class' : 'value'} |
There was a problem hiding this comment.
This hard-codes every non-classes use of the generalized dialog to the singular value. If the new itemNounPlural prop is reused for another categorical source, for example tags, the manual tab will render copy like No value found. while the rest of the dialog says tags. Add a matching singular prop instead of deriving it from one special case.
AGENTS.md reference: AGENTS.md:L39-L41
Useful? React with 👍 / 👎.
…rical values Adds itemNounPlural/showCountMode props so both components work for annotation classes and categorical metadata values. Adds the selectVisibleCounts helper and extends BarChart/types with orientation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
52599bb to
09a545f
Compare
4ea245b to
0d8fd50
Compare
1 participant
Summary
Stacked on p10.
Makes the existing config components reusable for categorical metadata values (not just annotation classes):
ClassSetConfigDialog/ManualClassSelector: accept anitemNounPluralprop ("classes"by default,"values"for categorical use) — labels like "Search classes…" and "No class found." become context-sensitiveDistributionConfigDialog: acceptsitemNounPluralandshowCountModeprops so the count-mode selector is hidden for categorical distributions (which have no object/frame distinction)selectVisibleCountshelper: standalone pure function that slices aCategoryCount[]array to the configured top-N limit — extracted here so the panel can use it independentlyBarChart/types.ts: addsorientationtoBarChartConfigfor persistenceDatasetDistributionPanel/types.ts: addsCategoricalDistributionConfigand extendsDistributionSourceGroupTest plan
make static-checks(frontend) passesnpm run test:unitpasses (updated tests inClassSetConfigDialog,ManualClassSelector,DistributionConfigDialog,selectVisibleCounts)Part of LIG-9585.