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.

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

[Accessibility] Add button to skip past the discover doc table #12539

Merged

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented Jun 28, 2017

Release Note: The new "Skip to bottom"-button in Discover is invisible unless focused. When activated it displays all remaining rows that were already loaded and focuses an anchor located below the table.

fixes #11855

image

@weltenwort weltenwort self-assigned this Jun 28, 2017
@Bargs
Copy link
Contributor

Bargs commented Jun 28, 2017

Let's try to hide it, if it ends up being too complicated we'll leave it visible.

@cjcenizal
Copy link
Contributor

cjcenizal commented Jun 30, 2017

@weltenwort Would you mind requesting @aphelionz for a review for you're ready?

The "Skip to bottom"-button displays all remaining rows that were
already loaded and focuses an anchor located below the table.
@weltenwort weltenwort force-pushed the fix/doctable-accessibility-load-all branch from 9e8e289 to a19391f Compare July 3, 2017 12:13
@weltenwort weltenwort added review and removed WIP Work in progress labels Jul 3, 2017
Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

Made a couple of comments down below.

$scope.scrollToBottom = function () {
// delay scrolling to after the rows have been rendered
$timeout(() => {
$document.find('#discoverBottomMarker').focus();
Copy link
Member

Choose a reason for hiding this comment

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

Any reason you're opting to use $document instead of $element here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's just a remnant of the fact that the code lived deeper in the directive hierarchy previously. good catch

<div ng-if="rows.length == opts.sampleSize" class="discover-table-footer">
These are the first {{opts.sampleSize}} documents matching
your search, refine your search to see others.
<a ng-click="toTop()">Back to top.</a>
<a tabindex="0" ng-click="scrollToTop()">Back to top.</a>
Copy link
Member

Choose a reason for hiding this comment

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

Since you're in here anyway, would you mind adding kbnAccessibleClick to this element? Currently if it has focus and you hit enter, nothing happens.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doing that will also mean you can remove tabindex="0".

@weltenwort
Copy link
Member Author

@lukasolson @cjcenizal Thanks for the reviews! I have addressed the two issue you mentioned.

@weltenwort
Copy link
Member Author

would you mind taking another look, @lukasolson @cjcenizal @aphelionz

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@aphelionz aphelionz left a comment

Choose a reason for hiding this comment

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

👍

@weltenwort
Copy link
Member Author

jenkins, test this

@weltenwort weltenwort merged commit b97a265 into elastic:master Jul 11, 2017
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.

Accessibility: Remove infinite scrolling from Discover table
7 participants