Skip to content
This repository was archived by the owner on Apr 25, 2025. It is now read-only.

feat(table): added loading body state#2453

Merged
agliga merged 7 commits into15.2.0from
table-loading
Mar 27, 2025
Merged

feat(table): added loading body state#2453
agliga merged 7 commits into15.2.0from
table-loading

Conversation

@agliga
Copy link
Copy Markdown
Contributor

@agliga agliga commented Mar 12, 2025

Description

  • Added table loading state. Used body-state. Was looking at using mode initially but that can remove selected state (can have load + selected`)
  • Added isLoading variable to disable all interactive elements in template
  • Added check in component to check all focusable elements (IE elements which are added by user) and disable/add tabindex -1 for them.
  • For undo only remove tabindex and disabled attribute for items found in the list of disabled items
  • Added tests for checking loading state

References

#2328

Screenshots

Screenshot 2025-03-11 at 4 04 19 PM

@agliga agliga self-assigned this Mar 12, 2025
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 12, 2025

🦋 Changeset detected

Latest commit: 2faad36

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@ebay/ebayui-core Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

}

onMount() {
this.disabledItems = new Map();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe a Set is better here since we just need to check if it is in the list and we are always setting as true.
Should we move this to the setLoading() function instead so it doesn't depend on the lifecycle initialization.
SOmething like:

setLoading() {
  this.disabledItem ??= new Set()
  const tbody = this.getEl('tbody')
}

setLoading() {
if (this.input.bodyState === "loading") {
if (this.tbody) {
requestAnimationFrame(() => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should call cancel request animation frame on unmount and on update.

Copy link
Copy Markdown
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

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

This looks to be missing a lot of the requirements for a fully disabled table in loading state.

Table in Loading State
When a table is fetching data for sorting, filtering, or pagination, a loading state should be displayed to indicate that the table is in the process of loading. This can be achieved by adding the table--loading-state class to the .table element.

Additionally, in the loading state, the module should be inaccessible to the user. This can be achieved by setting tabindex="-1" to the module container, and adding the inert attribute to the table inside the module. Since inert is not fully supported in older browsers, it is recommended to either use a polyfill or use JavaScript to disable all interactive elements inside the table when in loading state. Elements that support disabled should be disabled. Anchors should have their href attributes removed.

Here are some other steps to manually ensure the module is inaccessible:

All interactive elements inside table should be forced to NOT be focusable.
The module container should be forced to be unscrollable.
The progress bar should be forced to gain focus.
All interactive elements inside the table should be disabled.
The loading state should be displayed until the data is fully loaded. In addition to the loading state being set on the table, a progress bar expressive should be added to relay the loading state both visually and for screen readers. The live examples that follow use aria-live="off" to prevent the status element from proactively announcing new content. This is only for demo purposes, so that this documentation page does not overwhelm the viewer with example status updates. The aria-live override should not be included in implementations of this pattern.

https://opensource.ebay.com/skin/component/table/

@agliga
Copy link
Copy Markdown
Contributor Author

agliga commented Mar 20, 2025

This looks to be missing a lot of the requirements for a fully disabled table in loading state.

Table in Loading State

When a table is fetching data for sorting, filtering, or pagination, a loading state should be displayed to indicate that the table is in the process of loading. This can be achieved by adding the table--loading-state class to the .table element.

Additionally, in the loading state, the module should be inaccessible to the user. This can be achieved by setting tabindex="-1" to the module container, and adding the inert attribute to the table inside the module. Since inert is not fully supported in older browsers, it is recommended to either use a polyfill or use JavaScript to disable all interactive elements inside the table when in loading state. Elements that support disabled should be disabled. Anchors should have their href attributes removed.

Here are some other steps to manually ensure the module is inaccessible:

All interactive elements inside table should be forced to NOT be focusable.

The module container should be forced to be unscrollable.

The progress bar should be forced to gain focus.

All interactive elements inside the table should be disabled.

The loading state should be displayed until the data is fully loaded. In addition to the loading state being set on the table, a progress bar expressive should be added to relay the loading state both visually and for screen readers. The live examples that follow use aria-live="off" to prevent the status element from proactively announcing new content. This is only for demo purposes, so that this documentation page does not overwhelm the viewer with example status updates. The aria-live override should not be included in implementations of this pattern.

https://opensource.ebay.com/skin/component/table/

Can you be specific which requirements are missing?
The non interactive Elements are present (adding tabindex=-1)
Not sure what else is needed.

Copy link
Copy Markdown
Contributor

@saiponnada saiponnada left a comment

Choose a reason for hiding this comment

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

Overall LGTM. minor suggestions.

@agliga
Copy link
Copy Markdown
Contributor Author

agliga commented Mar 24, 2025

Should be ready for review. Again, if there are any gaps in what skin has, please comment what is missing because I did follow those guidelines on this.

@ArtBlue
Copy link
Copy Markdown
Contributor

ArtBlue commented Mar 25, 2025

Here's Skin:

image

A couple of things I noticed right away:

  1. The div.table doesn't have tabindex="-1"
  2. The inert is not set.
  3. The link sorting can't disable anchors since <a> is not something that be disabled. What we need for anchors is to be removed.
    image
    image
  4. To avoid having to keep track of all the anchor URLs before removing them and then resetting them, I'd suggest we simply change the attributes from href to data-href during loading, and when loading completed, changing them back to href.

@agliga
Copy link
Copy Markdown
Contributor Author

agliga commented Mar 25, 2025

Here's Skin:

image

A couple of things I noticed right away:

  1. The div.table doesn't have tabindex="-1"
  2. The inert is not set.
  3. The link sorting can't disable anchors since <a> is not something that be disabled. What we need for anchors is to be removed.
    image
    image
  4. To avoid having to keep track of all the anchor URLs before removing them and then resetting them, I'd suggest we simply change the attributes from href to data-href during loading, and when loading completed, changing them back to href.

Updated 3 and 4

I left out 1 and 2 since they aren't really needed.

Copy link
Copy Markdown
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

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

Looks great! @saiponnada , does this look good from the voiceover perspective?

@saiponnada saiponnada self-requested a review March 27, 2025 16:40
Copy link
Copy Markdown
Contributor

@saiponnada saiponnada left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for taking time and making these changes.

@agliga agliga merged commit 54351d9 into 15.2.0 Mar 27, 2025
5 checks passed
@agliga agliga deleted the table-loading branch March 27, 2025 17:00
This was referenced Mar 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants