Skip to content

[UIK-5053][data-table] fixed stories with checkboxes#2837

Open
ilyabrower wants to merge 12 commits intorelease/v17from
UIK-5053/old-selectedRows-type-deprecation
Open

[UIK-5053][data-table] fixed stories with checkboxes#2837
ilyabrower wants to merge 12 commits intorelease/v17from
UIK-5053/old-selectedRows-type-deprecation

Conversation

@ilyabrower
Copy link
Copy Markdown
Contributor

@ilyabrower ilyabrower commented Apr 2, 2026

Motivation and Context

How has this been tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • Nice improve.

Checklist:

  • I have updated the documentation accordingly.
  • I have added new tests on added of fixed functionality.

@ilyabrower ilyabrower self-assigned this Apr 2, 2026
@ilyabrower ilyabrower force-pushed the UIK-5053/old-selectedRows-type-deprecation branch from 57e4c88 to 98db3e0 Compare April 9, 2026 08:14
scrollPaddingTop: '44px',
}}
>
<ScreenReaderOnly role='status' aria-live='polite'>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this shouldn't be removed from this example, the text just need to be slightly different: "Actions are available before the table" (btw this exact pattern is used in the example we're currently testing, and the participant said the announcement was very convenient)

Copy link
Copy Markdown
Contributor 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, that we need it in advanced example, but if you insist, I'll put it back

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's add it, this code can be copied/referenced as well, it's better to be accessible

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed this old example, and added SR for the reactive example (your next comment)

const unsubscribe = selectedRows.subscribe(SelectableRows.TOGGLE_EVENT, () => {
const selectedRowsSize = selectedRows.get().length;
if (selectedRowsSize > 0) {
setAriaMessage('Action bar appeared before the table');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same - we need to keep the behavior, just change the text

const handleChangeSelectedRows = (value: string[]) => {
setSelectedRows(value);
if (!selectedRows.length) setAriaMessage('Action bar appeared before the table');
if (value.length) setSelectedRowsDisplay(value.length);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

aria message now is announced every time selection changes (previously there was this logic to only set the aria message if the previous value was zero)

But, I think instead of getting back the old solution, we can do it slightly differently:

  • change the text to "Actions are available before the table" (like in the other examples)
  • remove the logic clearing the message after timeout. Clear the message only if selected rows length = 0. It's ok that user might stumble upon the message when navigating through the page - because now the text is neutral and appropriate even for that case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

changed to your suggestion

@ilyabrower ilyabrower force-pushed the release/v17 branch 6 times, most recently from 919f006 to 94f6b31 Compare April 14, 2026 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants