Skip to content

Commit d4e7658

Browse files
MelvinBot{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}
andcommitted
Scope RHP Table selection behind opt-in prop
The previous commit changed the shared Table's selection gating from shouldUseNarrowLayout to isSmallScreenWidth globally, which unintentionally changed behavior for other Table consumers rendered in the RHP (Categories, Tags, and Expense Rules pages) — they would show selection checkboxes on wide-desktop RHP where they previously did not. Gate the new behavior behind a shouldEnableSelectionInNarrowPaneModal prop (default false), which the ReportParticipantsTable opts into. With the prop off, the selection UX reduces exactly to the original shouldUseNarrowLayout behavior, so every other consumer is unaffected. Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
1 parent 5a7ca47 commit d4e7658

7 files changed

Lines changed: 43 additions & 20 deletions

File tree

src/components/Table/Table.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,7 @@ function Table<DataType extends TableData, ColumnKey extends string = string, Fi
153153
narrowLayoutSortColumn,
154154
children,
155155
selectionEnabled,
156+
shouldEnableSelectionInNarrowPaneModal,
156157
onRowSelectionChange,
157158
...listProps
158159
}: TableProps<DataType, ColumnKey, FilterKey>) {
@@ -190,7 +191,7 @@ function Table<DataType extends TableData, ColumnKey extends string = string, Fi
190191
methods: selectionMethods,
191192
mobileSelectionModalRowKey,
192193
middleware: selectionMiddleware,
193-
} = useSelection<DataType>({data: sortedData, originalSelectableCount, currentFilters, selectedKeys, onRowSelectionChange});
194+
} = useSelection<DataType>({data: sortedData, originalSelectableCount, currentFilters, selectedKeys, onRowSelectionChange, shouldEnableSelectionInNarrowPaneModal});
194195
const selectionData = selectionMiddleware(sortedData);
195196

196197
const {methods: highlightingMethods, middleware: highlightMiddleware} = useHighlighting<DataType>();
@@ -256,6 +257,7 @@ function Table<DataType extends TableData, ColumnKey extends string = string, Fi
256257
isEmptyResult,
257258
shouldUseNarrowTableLayout,
258259
selectionEnabled,
260+
shouldEnableSelectionInNarrowPaneModal,
259261
isMobileSelectionEnabled,
260262
};
261263

src/components/Table/TableContext.tsx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,9 @@ type TableContextValue<DataType extends TableData, ColumnKey extends string = st
2424
/** Whether or not selection is enabled for the table */
2525
selectionEnabled?: boolean;
2626

27+
/** Whether the selection UX should key off the real screen size instead of shouldUseNarrowLayout (for tables inside a narrow pane modal / RHP) */
28+
shouldEnableSelectionInNarrowPaneModal?: boolean;
29+
2730
/** The data array after filtering, searching, and sorting have been applied. */
2831
processedData: Array<TableRow<DataType>>;
2932

src/components/Table/TableHeader.tsx

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,12 +53,14 @@ function TableHeader<DataType extends TableData, ColumnKey extends string = stri
5353
const theme = useTheme();
5454
const styles = useThemeStyles();
5555
const {translate} = useLocalize();
56-
// isSelectionCheckboxVisible keys off isSmallScreenWidth (real mobile) rather than shouldUseNarrowLayout so the
57-
// header checkbox renders when the table is inside a narrow pane modal (RHP). The visual padding below still uses shouldUseNarrowLayout.
5856
// eslint-disable-next-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth
5957
const {shouldUseNarrowLayout, isSmallScreenWidth} = useResponsiveLayout();
60-
const {columns, isEmptyResult, title, shouldUseNarrowTableLayout, tableMethods, selectionEnabled, processedData, isMobileSelectionEnabled} = useTableContext<DataType, ColumnKey>();
61-
const isSelectionCheckboxVisible = selectionEnabled && (isMobileSelectionEnabled || !isSmallScreenWidth);
58+
const {columns, isEmptyResult, title, shouldUseNarrowTableLayout, tableMethods, selectionEnabled, processedData, isMobileSelectionEnabled, shouldEnableSelectionInNarrowPaneModal} =
59+
useTableContext<DataType, ColumnKey>();
60+
// Tables inside a narrow pane modal (RHP) opt into keying the header checkbox off the real screen size, since
61+
// shouldUseNarrowLayout is always true in an RHP. Other tables keep the original behavior. Visual padding below still uses shouldUseNarrowLayout.
62+
const selectionUsesNarrowLayout = shouldEnableSelectionInNarrowPaneModal ? isSmallScreenWidth : shouldUseNarrowLayout;
63+
const isSelectionCheckboxVisible = selectionEnabled && (isMobileSelectionEnabled || !selectionUsesNarrowLayout);
6264

6365
if (shouldUseNarrowTableLayout && !title) {
6466
return null;

src/components/Table/TableRow.tsx

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -56,18 +56,20 @@ export default function TableRow({
5656
const theme = useTheme();
5757
const styles = useThemeStyles();
5858
const {translate} = useLocalize();
59-
// The selection UX keys off the real screen size (isSmallScreenWidth) rather than shouldUseNarrowLayout so that
60-
// long-press selection and checkboxes still work when the table is rendered inside a narrow pane modal (RHP),
61-
// where shouldUseNarrowLayout is always true regardless of screen size. Visual layout keeps using shouldUseNarrowTableLayout.
6259
// eslint-disable-next-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth
63-
const {isSmallScreenWidth} = useResponsiveLayout();
64-
const shouldEnableMobileSelectionLongPress = isSmallScreenWidth;
65-
const {processedData, columns, shouldUseNarrowTableLayout, tableMethods, selectionEnabled, isMobileSelectionEnabled} = useTableContext();
60+
const {isSmallScreenWidth, shouldUseNarrowLayout, isInNarrowPaneModal} = useResponsiveLayout();
61+
const {processedData, columns, shouldUseNarrowTableLayout, tableMethods, selectionEnabled, isMobileSelectionEnabled, shouldEnableSelectionInNarrowPaneModal} = useTableContext();
62+
63+
// Tables inside a narrow pane modal (RHP) opt into keying the selection UX off the real screen size (isSmallScreenWidth),
64+
// because shouldUseNarrowLayout is always true in an RHP and would otherwise suppress selection entirely. All other
65+
// tables keep the original shouldUseNarrowLayout behavior. Visual layout still uses shouldUseNarrowTableLayout.
66+
const selectionUsesNarrowLayout = shouldEnableSelectionInNarrowPaneModal ? isSmallScreenWidth : shouldUseNarrowLayout;
67+
const shouldEnableMobileSelectionLongPress = isSmallScreenWidth && (shouldEnableSelectionInNarrowPaneModal || !isInNarrowPaneModal);
6668

6769
const item = processedData.at(rowIndex);
6870
const rowCount = processedData.length;
6971
const gridTemplateColumns = columns.map((column) => (column.width ? `${column.width}px` : '1fr'));
70-
const isSelectionCheckboxVisible = selectionEnabled && (isMobileSelectionEnabled || !isSmallScreenWidth);
72+
const isSelectionCheckboxVisible = selectionEnabled && (isMobileSelectionEnabled || !selectionUsesNarrowLayout);
7173

7274
const isDisabled = !!disabled;
7375
const isFirstRow = rowIndex === 0;
@@ -149,7 +151,7 @@ export default function TableRow({
149151
return;
150152
}
151153

152-
if (!isSmallScreenWidth || !isMobileSelectionEnabled || !selectionEnabled) {
154+
if (!selectionUsesNarrowLayout || !isMobileSelectionEnabled || !selectionEnabled) {
153155
onPress?.();
154156
return;
155157
}

src/components/Table/middlewares/selection.ts

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@ type UseSelectionProps<DataType extends TableData> = {
2323

2424
/** Callback that is fired when the selection of rows in the table changes */
2525
onRowSelectionChange?: (selectedRowKeys: string[]) => void;
26+
27+
/** Whether the selection mode should key off the real screen size instead of shouldUseNarrowLayout (for tables inside a narrow pane modal / RHP) */
28+
shouldEnableSelectionInNarrowPaneModal?: boolean;
2629
};
2730

2831
type SelectionMethods = {
@@ -53,12 +56,14 @@ export default function useSelection<DataType extends TableData>({
5356
selectedKeys,
5457
currentFilters,
5558
onRowSelectionChange,
59+
shouldEnableSelectionInNarrowPaneModal,
5660
}: UseSelectionProps<DataType>): UseSelectionResult<DataType> {
57-
// The selection-mode auto-sync keys off isSmallScreenWidth (real mobile) rather than shouldUseNarrowLayout so it
58-
// behaves correctly when the table is rendered inside a narrow pane modal (RHP), where shouldUseNarrowLayout is
59-
// always true. On a wide screen the RHP shows checkboxes directly instead of the mobile long-press selection mode.
61+
// When a table opts into selection inside a narrow pane modal (RHP), the selection-mode auto-sync keys off the real
62+
// screen size (isSmallScreenWidth) so it behaves correctly there (shouldUseNarrowLayout is always true in an RHP).
63+
// Otherwise it keeps the original shouldUseNarrowLayout behavior, so central-pane tables are unaffected.
6064
// eslint-disable-next-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth
61-
const {isSmallScreenWidth} = useResponsiveLayout();
65+
const {shouldUseNarrowLayout, isSmallScreenWidth} = useResponsiveLayout();
66+
const selectionUsesNarrowLayout = shouldEnableSelectionInNarrowPaneModal ? isSmallScreenWidth : shouldUseNarrowLayout;
6267
const isSelectionModeEnabled = useMobileSelectionMode();
6368
const lastSelectedRowKeyRef = useRef<string | null>(null);
6469
const lastSelectedRowIsSelectedRef = useRef<boolean>(false);
@@ -89,16 +94,16 @@ export default function useSelection<DataType extends TableData>({
8994

9095
// Sync the selection mode with the screen size & selection state
9196
useEffect(() => {
92-
const isMobileMissingSelectionMode = isSmallScreenWidth && !isSelectionModeEnabled && selectedKeys.length;
93-
const isDesktopWithoutSelectableKeys = isSelectionModeEnabled && !selectableKeys.length && !isSmallScreenWidth;
97+
const isMobileMissingSelectionMode = selectionUsesNarrowLayout && !isSelectionModeEnabled && selectedKeys.length;
98+
const isDesktopWithoutSelectableKeys = isSelectionModeEnabled && !selectableKeys.length && !selectionUsesNarrowLayout;
9499
const isSelectionModeEnabledWithoutSelectableKeys = isSelectionModeEnabled && !selectableKeys.length && !originalSelectableCount;
95100

96101
if (isMobileMissingSelectionMode) {
97102
turnOnMobileSelectionMode();
98103
} else if (isDesktopWithoutSelectableKeys || isSelectionModeEnabledWithoutSelectableKeys) {
99104
turnOffMobileSelectionMode();
100105
}
101-
}, [isSmallScreenWidth, isSelectionModeEnabled, selectedKeys.length, originalSelectableCount, selectableKeys.length]);
106+
}, [selectionUsesNarrowLayout, isSelectionModeEnabled, selectedKeys.length, originalSelectableCount, selectableKeys.length]);
102107

103108
// When selection mode is turned off, clear the list of selected keys, so that re-enabling selection mode doesn't retain rows
104109
const wasSelectionModeEnabled = usePrevious(isSelectionModeEnabled);

src/components/Table/types.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,14 @@ type TableProps<DataType extends TableData, ColumnKey extends string = string, F
146146
/** Whether multi selection is enabled */
147147
selectionEnabled?: boolean;
148148

149+
/**
150+
* Whether the selection UX (checkboxes / long-press selection mode) should be driven by the real screen size
151+
* (isSmallScreenWidth) instead of shouldUseNarrowLayout. Set this for tables rendered inside a narrow pane modal
152+
* (RHP), where shouldUseNarrowLayout is always true and would otherwise suppress selection entirely. Defaults to
153+
* false so central-pane tables keep their existing behavior.
154+
*/
155+
shouldEnableSelectionInNarrowPaneModal?: boolean;
156+
149157
/** Column configuration defining what columns to display and how. */
150158
columns: Array<TableColumn<ColumnKey>>;
151159

src/components/Tables/ReportParticipantsTable/index.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ export default function ReportParticipantsTable({ref, members, isGroupChat, sele
111111
columns={columns}
112112
selectedKeys={selectedKeys}
113113
selectionEnabled={selectionEnabled}
114+
shouldEnableSelectionInNarrowPaneModal
114115
initialSortColumn="member"
115116
title={translate('common.members')}
116117
renderItem={renderItem}

0 commit comments

Comments
 (0)