bito-code-review[bot] commented on code in PR #42009:
URL: https://github.com/apache/superset/pull/42009#discussion_r3589752201
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/components/CustomHeader.tsx:
##########
@@ -201,17 +205,35 @@ const CustomHeader: React.FC<CustomHeaderParams> = ({
const menuContent = (
<MenuContainer>
{shouldShowAsc && (
- <div onClick={() => applySort('asc')} className="menu-item">
+ <div
+ role="button"
+ tabIndex={0}
+ onClick={() => applySort('asc')}
+ onKeyDown={handleKeyboardActivation(() => applySort('asc'))}
+ className="menu-item"
+ >
<ArrowUpOutlined /> {t('Sort Ascending')}
</div>
)}
{shouldShowDesc && (
- <div onClick={() => applySort('desc')} className="menu-item">
+ <div
+ role="button"
+ tabIndex={0}
+ onClick={() => applySort('desc')}
+ onKeyDown={handleKeyboardActivation(() => applySort('desc'))}
+ className="menu-item"
+ >
<ArrowDownOutlined /> {t('Sort Descending')}
</div>
)}
{currentSort && currentSort?.colId === colId && (
- <div onClick={clearSort} className="menu-item">
+ <div
+ role="button"
+ tabIndex={0}
+ onClick={clearSort}
+ onKeyDown={handleKeyboardActivation(clearSort)}
+ className="menu-item"
+ >
<span style={{ fontSize: 16 }}>↻</span> {t('Clear Sort')}
</div>
)}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Repeated button pattern duplicated 3x</b></div>
<div id="fix">
The three sort-menu items (Ascending, Descending, Clear Sort) each repeat
the identical JSX pattern for keyboard-accessible buttons. This semantic
duplication creates maintenance risk — any future change (e.g., adding
aria-label) would need to be applied consistently to all three locations.
</div>
</div>
<small><i>Code Review Run #b9f559</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopoverTitle.tsx:
##########
@@ -120,6 +121,7 @@ const AdhocMetricEditPopoverTitle:
FC<AdhocMetricEditPopoverTitleProps> = ({
onMouseOut={handleMouseOut}
onFocus={handleMouseOver}
onClick={handleClick}
+ onKeyDown={handleKeyboardActivation(handleClick)}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing keyboard activation test</b></div>
<div id="fix">
The new keyboard activation handler on line 124 is correctly implemented,
but the existing test `'start and end the title edit mode'` only covers mouse
clicks, not the new keyboard activation path. Add a test using
`fireEvent.keyDown` on the trigger to verify pressing Enter or Space activates
edit mode, ensuring the accessibility improvement is fully covered by tests.
</div>
</div>
<small><i>Code Review Run #b9f559</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
