FE: categorical distribution orientation toggle and spacing polish - #1739
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 pass
To run the guardrails locally, from 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: 007379b688
ℹ️ 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".
| setCategoricalConfig({ | ||
| ...categoricalConfig, | ||
| orientation: |
There was a problem hiding this comment.
When a categorical field is still using its default config, this stores the whole categoricalConfig just to change orientation, including n as the current categoricalData.length. If that same field later refetches with more buckets, for example after clearing filters that previously narrowed it to a few values, selectVisibleCounts will keep showing only the old top-N even though the user only toggled orientation. Update only the orientation or keep the default n dynamic until the user actually changes the visible-value config.
Useful? React with 👍 / 👎.
| {totalCount} | ||
| onBarClick={activeCategorical ? handleCategoricalBarClick : onBarClick} | ||
| emptyState={activeCategorical ? categoricalEmptyState : undefined} | ||
| gridTopPx={4} |
There was a problem hiding this comment.
This raw spacing value is now repeated in the panel BarChart call, the expanded dialog, histogram grid setup, and tests, so future spacing tweaks require hunting down several unrelated 4s and the intent is easy to lose. Please define a shared named constant for the compact chart top padding and reuse it across these chart variants.
AGENTS.md reference: AGENTS.md:L39-L41
Useful? React with 👍 / 👎.
7a34385 to
c2eea76
Compare
007379b to
14054f3
Compare
1 participant
Summary
Stacked on p13. Polish and UX — no new data fetching or filter logic.
Spacing polish (
BarChart.svelte,DatasetDistributionPanel,ExpandDialog,PanelHeader,Histogram):BarChartgains an explicitheightprop instead of relying on implicit container sizingOrientation toggle (
DatasetDistributionPanel,ExpandDialog,PanelHeader):ExpandDialogmirrors the orientation from the panel configNormalization (
DatasetDistributionPanel,ExpandDialog,PanelHeader): unified spacing constants so both numeric and categorical distribution groups look consistent.Test plan
make static-checks(frontend) passesnpm run test:unitpasses (updated render tests forDatasetDistributionPanel,ExpandDialog,PanelHeader,buildHistogramOption)Part of LIG-9585.