fix(table-chart): fix "Search by" control visibility and improve table controls layout #36073
Conversation
Code Review Agent Run #1c82ecActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
github-actions
bot
added
i18n
dosubot
bot
added
change:frontend
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Missing right margin in CSS refactor ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/plugins/plugin-chart-table/src/DataTable/components/SearchSelectDropdown.tsx | ✅ |
| superset-frontend/plugins/plugin-chart-table/src/DataTable/components/GlobalFilter.tsx | ✅ |
| superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx | ✅ |
| superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
superset-frontend/plugins/plugin-chart-table/src/DataTable/components/SearchSelectDropdown.tsx
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #36073 +/- ##
===========================================
+ Coverage 0 68.14% +68.14%
===========================================
Files 0 632 +632
Lines 0 46450 +46450
Branches 0 5035 +5035
===========================================
+ Hits 0 31652 +31652
- Misses 0 13536 +13536
- Partials 0 1262 +1262
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the "Search by" control visibility logic in the Table chart and improves the layout of table controls. The key change corrects when the "Search by" dropdown appears - it now only shows when both "Server pagination" AND "Search box" options are enabled, rather than appearing with just "Server pagination" alone. Additionally, the PR enhances the visual consistency by moving control labels above their inputs and standardizing font sizes.
- Corrects "Search by" control visibility to require both server pagination and search box to be enabled
- Refactors control layouts to use vertical spacing with labels positioned above inputs
- Standardizes control sizing and font sizes for better visual consistency with table content
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| superset/translations/ru/LC_MESSAGES/messages.po | Updates Russian translations: removes plural forms for "%s record", adds translations for "Entries per page" and "Search by" |
| superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx | Removes tn import, updates SearchInput and SelectPageSize to use vertical layout, changes label text and removes placeholder |
| superset-frontend/plugins/plugin-chart-table/src/DataTable/components/SearchSelectDropdown.tsx | Replaces styled component with inline CSS using theme tokens, adds size prop for consistency |
| superset-frontend/plugins/plugin-chart-table/src/DataTable/components/GlobalFilter.tsx | Adds translation function import and wraps "Search" text with t() for internationalization |
| superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx | Implements fixed visibility logic for "Search by" control (requires both serverPagination and searchInput), adds vertical layout with label, and applies font size styling |
Comments suppressed due to low confidence (1)
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:228
- The placeholder attribute was removed from the Input component when the
tnfunction call was removed. This means users will see an empty input field with no hint about what to search for. Consider adding a placeholder using thet()function, for example:placeholder={t('Search...')}or similar text to provide a better user experience.
<Input
aria-label={t('Search %s records', count)}
value={value}
size="small"
onChange={onChange}
onBlur={onBlur}
ref={inputRef}
/>
SBIN2010
left a comment
SBIN2010
left a comment
There was a problem hiding this comment.
I left a couple of comments. But overall, it looks pretty good.
| {t('Search')} | ||
| <Input | ||
| aria-label={t('Search %s records', count)} | ||
| placeholder={tn('%s record', '%s records...', count, count)} |
There was a problem hiding this comment.
The placeholder attribute has been removed from the Input component. This means users will see an empty input field with no indication of how many records are in the table. For a better user experience, I recommend leaving the placeholder.
There was a problem hiding this comment.
I see your concern but I'm not sure how showing the total record count in the search field improves the user experience.
That number doesn't help a user with their search. Whether there are 2 records or 2 million, they can still type a query and get zero results. The placeholder should help users search, and the total count doesn't do that.
superset-frontend/plugins/plugin-chart-table/src/DataTable/components/SearchSelectDropdown.tsx
Show resolved
Hide resolved
|
Looks like a couple failing tests, a conflict resolution needed, and a couple open comments on the thread, if you don't mind helping to get this through @innovark37. Thanks in advance! |
# Conflicts: # superset/translations/ru/LC_MESSAGES/messages.po
3 participants
SUMMARY
This PR addresses two main issues with the Table chart controls:
Layout Improvement: Enhances the visual layout of the "Entries per page", "Search by", and "Search" controls by moving labels above inputs and adjusting font sizes for better consistency with the table content.
Logic Fix: Corrects the display logic for the "Search by" control, which was previously shown incorrectly.
Now shows the control only when both "Server pagination" AND "Search box" options are enabled. Previously it appeared with only "Server Pagination" enabled, which was functionally incorrect.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE
AFTER
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION