Improve EuiBasicTable caption #2782

andreadelrio · 2020-01-22T01:28:10Z

Summary

Improve EuiBasicTable caption to include the number of total rows when there's multiple pages.

Previous caption

Below is a table of {itemCount} items.

New caption

Below is a table containing {itemCount} rows. [There's only 1 page]
Below is a table containing {itemCount} rows out of {totalItemCount}. [When there's more than 1 page]

Not sure what to do in the case shown below. Currently the caption for this table would be Below is a table containing 0 rows. Should we be doing something like Below is a table containing no data instead? @myasonik what do you think?

Fixes #2336

Checklist

~~- [ ] Check against all themes for compatibility in both light and dark modes~~
- [ ] Checked in mobile
- [ ] Checked in IE11 and Firefox
- [ ] Props have proper autodocs
~~- [ ] Added documentation examples~~

Added or updated jest tests
~~- [ ] Checked for breaking changes and labeled appropriately~~
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

myasonik · 2020-01-22T02:41:25Z

Sorry time chime in only after you've put some time into this but, honestly, I'm not sure if there's much/any value in this sort of caption.

Screen readers already have robust table navigation capabilities so describing the size of the table is usually already done natively. (This isn't the case for paginated tables but that requires a different solution outside of captions.)

Also, this quickly breaks down when you have multiple tables on a page because the caption gives you no context for what's in it.

I think the most useful default caption would be to list the column titles. Maybe something like This table contains info on: <column titles>? This avoids the syntax problem of below and provides more context as a title than the number of rows. (Yes, users can find out column headers natively, same as the can find out the number of rows, but I think that a list of column headers is a better "name" for a table than the number of rows it contains.)

But captions need to be overridable by implementing developers because the best captions will always usually be user supplied (until we can run sentiment analysis on tables). (For your example table here, something like Table of user data is more to the point than listing every column title.)

src/components/basic_table/basic_table.tsx

snide · 2020-01-22T21:47:34Z

@myasonik Can you distill that into what you'd like to see here? Specifically I agree that this is likely best served by the consuming engineer. My suggestion would be to:

Make the caption configurable, with the default left as @andreadelrio has it (i don't think putting a list of column names does much (not to mention unwieldy), and is better served by Improve accessibility of EuiTableRowCell #2335 which we can attack separately).
Put a docs note suggesting setting caption is important and people should use it!
Remove the aria/role bits on caption as you suggest.

That's a decent first step that allows folks to fix #2336 throughout Kibana, then we can consider this (and a couple other a11y required props) later in a holistic a11y breaking changes release in the future.

myasonik · 2020-01-22T22:16:03Z

I agree with 2 and 3 in your list.

I think maybe an example will better explain why I prefer a list of column headers over a count of rows:

Think of the <caption> more as a "name" for table. (I know it sounds incorrect but it's more closely what it acts like.) Which makes more sense for me to say:

"Hey, will you take a look at that table which has 6 rows?"
"Hey, will you take a look at that table which tells me the First Name, Last Name, Date of Birth, and Nationality?"

Option 1 is what EUI currently has. Option 2 is what I'm proposing. It's longer, for sure, but between those options, I think Option 2 is more clear.

#2335 which you mention tackles a different problem all together.

myasonik · 2020-01-22T22:17:22Z

You also mention adding this to a future breaking change. Is that to suggest that we force consuming engineers to provide a caption?

(I would be for that if that's what you're suggesting.)

snide · 2020-01-22T22:37:14Z

You also mention adding this to a future breaking change. Is that to suggest that we force consuming engineers to provide a caption?

(I would be for that if that's what you're suggesting.)

Correct. Ultimately, that's the real way to solve this one. This first phase just unblocks folks quickly.

andreadelrio · 2020-01-22T23:05:52Z

Make the caption configurable, with the default left as @andreadelrio has it (i don't think putting a list of column names does much (not to mention unwieldy), and is better served by Improve accessibility of EuiTableRowCell #2335 which we can attack separately).

@snide This mean adding a new prop to EuiBasicTable, correct?

andreadelrio · 2020-01-23T00:28:46Z

I've added a prop, called tableCaption to allow consumer to pass a custom caption if they don't want to use the default. Also removed the aria/role unneeded attributes.

Where do we think the docs note should go? Perhaps somewhere in here?

myasonik · 2020-01-23T00:40:01Z

It definitely doesn't hurt to mention it in there but I was going to say that I think the most important spot is the props documentation which you've already done 👍

@andreadelrio What do you think about changing the default to be column headers instead of number of rows?

@barlowm you might have a good suggestion here for a default as you're the one that's been auditing all these tables

andreadelrio · 2020-01-25T01:53:02Z

@myasonik I've tried adding the names of columns as the default caption. Here's what I currently have:

Note: I initially tried to use column.name instead of column.field but found that to be an issue when the consumer defines the column.name doing something like this:

Let me know what you think.

myasonik · 2020-01-26T05:15:45Z

I see what you mean with something like that getting tricky... This isn't ideal but is maybe ok...

@chandlerprall - I wonder if you can help out here. I'm foggy on the details now but I recall you showing me something hidden in EUI to get rendered text that we used for some a11y feature somewhere, I think... Could we use that here?

snide · 2020-01-27T18:57:15Z

@myasonik I think you're referencing this https://elastic.github.io/eui/#/utilities/inner-text

I think we're getting too complicated here for what is still a fairly poor default. Let's remove the columns, leave it with the ability to set the caption manually and make a new issue to revisit this concept later. That unblocks folks. We can always get more complicated later. End of day the best, suggested option is to always write it manually.

andreadelrio · 2020-01-27T19:48:54Z

@myasonik I agree with Dave that this is getting too complicated. I've gone back to having the number of rows as the default caption. I changed it a little bit to remove the use of the word "below" (as you suggested).

snide

Small docs issue. LGTM.

snide · 2020-01-27T19:51:46Z

src-docs/src/views/tables/basic/props_info.js

@@ -95,6 +95,12 @@ export const propsInfo = {
          required: false,
          type: { name: '(criteria: #Criteria) => void' },
        },
+        tableCaption: {
+          description:
+            'Describes the content of the table. If not specified, the caption will be "Below is a table of {itemCount} items."',


Gonna want to adjust your docs copy to match your recent changes.

myasonik · 2020-01-29T18:17:51Z

src/components/basic_table/basic_table.tsx

+        <EuiI18n
+          token="euiBasicTable.tableDescription"
+          default="{tableCaption}"
+          values={{
+            tableCaption: tableCaption,
+          }}
+        />


Is this necessary? Usually the consuming app handles translation if it wants it and passes in a translated string.

thompsongl · 2020-01-29T19:50:52Z

src/components/basic_table/basic_table.tsx

+          <EuiI18n
+            token="euiBasicTable.tableDescription"
+            default="This table contains {itemCount} rows out of {totalItemCount} rows."
+            values={{
+              totalItemCount: pagination.totalItemCount,
+              itemCount: items.length,
+            }}
+          />
+        );
+      } else {
+        captionElement = (
+          <EuiI18n
+            token="euiBasicTable.tableDescription"
+            default="This table contains {itemCount} rows."
+            values={{
+              itemCount: items.length,
+            }}
+          />


These 2 EuiI8ns can't have the same token value because they have different default strings.
Just need to change one or both of their token to something else, like euiBasicTable.tableDescriptionWithPagination/euiBasicTable.tableDescriptionWithoutPagination

kobelb · 2020-01-29T23:30:14Z

Thanks for adding this @andreadelrio 🍾

improve EuiBasicTable caption

5757dc8

andreadelrio requested a review from myasonik January 22, 2020 01:28

cl

449d508

myasonik reviewed Jan 22, 2020

View reviewed changes

src/components/basic_table/basic_table.tsx Outdated Show resolved Hide resolved

snide mentioned this pull request Jan 22, 2020

[DISCUSS] A11y work and breaking changes friction #2784

Closed

andreadelrio added 2 commits January 22, 2020 16:21

Merge remote-tracking branch 'upstream/master' into basic-table-caption

49e571d

added tableCaption prop and updated cl

d89a6c9

andreadelrio added 2 commits January 24, 2020 17:36

changed default caption to list column names

d79f679

updated cl

84b2998

listing number of rows as caption

2fa4d05

andreadelrio requested a review from myasonik January 27, 2020 19:49

snide approved these changes Jan 27, 2020

View reviewed changes

updated prop description in docs

bd29385

myasonik approved these changes Jan 29, 2020

View reviewed changes

andreadelrio added 2 commits January 29, 2020 10:45

Merge remote-tracking branch 'upstream/master' into basic-table-caption

d7e5b05

removed i18n from tableCaption

247cbe1

thompsongl reviewed Jan 29, 2020

View reviewed changes

fixing tests

153cdf0

andreadelrio merged commit 31249e5 into elastic:master Jan 29, 2020

thompsongl mentioned this pull request Feb 11, 2020

Upgrade EUI to v19.0.0 elastic/kibana#57284

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve EuiBasicTable caption #2782

Improve EuiBasicTable caption #2782

andreadelrio commented Jan 22, 2020 •

edited by snide

Loading

myasonik commented Jan 22, 2020 •

edited

Loading

snide commented Jan 22, 2020

myasonik commented Jan 22, 2020 •

edited

Loading

myasonik commented Jan 22, 2020

snide commented Jan 22, 2020

andreadelrio commented Jan 22, 2020

andreadelrio commented Jan 23, 2020

myasonik commented Jan 23, 2020

andreadelrio commented Jan 25, 2020

myasonik commented Jan 26, 2020

snide commented Jan 27, 2020

andreadelrio commented Jan 27, 2020

snide left a comment

snide Jan 27, 2020

myasonik Jan 29, 2020

thompsongl Jan 29, 2020 •

edited

Loading

kobelb commented Jan 29, 2020

Improve EuiBasicTable caption #2782

Improve EuiBasicTable caption #2782

Conversation

andreadelrio commented Jan 22, 2020 • edited by snide Loading

Summary

Checklist

myasonik commented Jan 22, 2020 • edited Loading

snide commented Jan 22, 2020

myasonik commented Jan 22, 2020 • edited Loading

myasonik commented Jan 22, 2020

snide commented Jan 22, 2020

andreadelrio commented Jan 22, 2020

andreadelrio commented Jan 23, 2020

myasonik commented Jan 23, 2020

andreadelrio commented Jan 25, 2020

myasonik commented Jan 26, 2020

snide commented Jan 27, 2020

andreadelrio commented Jan 27, 2020

snide left a comment

Choose a reason for hiding this comment

snide Jan 27, 2020

Choose a reason for hiding this comment

myasonik Jan 29, 2020

Choose a reason for hiding this comment

thompsongl Jan 29, 2020 • edited Loading

Choose a reason for hiding this comment

kobelb commented Jan 29, 2020

andreadelrio commented Jan 22, 2020 •

edited by snide

Loading

myasonik commented Jan 22, 2020 •

edited

Loading

myasonik commented Jan 22, 2020 •

edited

Loading

thompsongl Jan 29, 2020 •

edited

Loading