Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(datatable): remove toolbar search and menu from tab order #14397

Merged
merged 3 commits into from
Aug 4, 2023
Merged

fix(datatable): remove toolbar search and menu from tab order #14397

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0bffd2f
fix(datatable): remove toolbar search and menu from tab order when ba…
tay1orjones Aug 4, 2023
3751fb9
chore: update snaps
tay1orjones Aug 4, 2023
cd8fb6b
Merge branch 'main' into 14357-datatable-focus-improvements
tay1orjones Aug 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions e2e/components/DataTable/DataTable-test.avt.e2e.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,6 @@ test.describe('DataTable @avt', () => {
await page.keyboard.press('Shift+Tab');
await page.keyboard.press('Shift+Tab');
await page.keyboard.press('Shift+Tab');
await page.keyboard.press('Shift+Tab');
await page.keyboard.press('Shift+Tab');

await page
.getByRole('heading', { name: 'Expandable row content' })
Expand Down
20 changes: 17 additions & 3 deletions ...act/src/components/DataTable/stories/dynamic-content/DataTable-dynamic-content.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,16 @@ export const Default = () => {
</TableBatchAction>
</TableBatchActions>
<TableToolbarContent>
<TableToolbarSearch onChange={onInputChange} />
<TableToolbarMenu>
<TableToolbarSearch
tabIndex={
batchActionProps.shouldShowBatchActions ? -1 : 0
}
onChange={onInputChange}
/>
<TableToolbarMenu
tabIndex={
batchActionProps.shouldShowBatchActions ? -1 : 0
}>
<TableToolbarAction onClick={this.handleOnRowAdd}>
Add row
</TableToolbarAction>
Expand Down Expand Up @@ -328,8 +336,14 @@ export const Playground = (args) => {
action('TableToolbarSearch - onChange')(evt);
onInputChange(evt);
}}
tabIndex={
batchActionProps.shouldShowBatchActions ? -1 : 0
}
/>
<TableToolbarMenu>
<TableToolbarMenu
tabIndex={
batchActionProps.shouldShowBatchActions ? -1 : 0
}>
<TableToolbarAction
onClick={(evt) => {
action('handleOnRowAdd')(evt);
Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/components/Search/Search.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,6 @@ const Search = React.forwardRef<HTMLInputElement, SearchProps>(function Search(
{labelText}
</label>
<input
{...rest}
autoComplete={autoComplete}
className={`${prefix}--search-input`}
defaultValue={defaultValue}
Expand All @@ -257,6 +256,7 @@ const Search = React.forwardRef<HTMLInputElement, SearchProps>(function Search(
type={type}
value={value}
tabIndex={onExpand && !isExpanded ? -1 : undefined}
{...rest}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why we had this placed before all the existing props. Props passed by consumers would always be overwritten by what we had here.

It's possible this could cause some unintended effects downstream, but I don't think it's necessarily a breaking change. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the only "unintended effect" that props users were passing and expecting to work will now actually work? 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is breaking either, more like a bug fix for something that should have been working.

/>
<button
aria-label={closeButtonLabelText}
Expand Down
Loading