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

LinkControl - adds search results label for initial suggestions #19665

Merged
merged 6 commits into from
Jan 15, 2020

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Jan 15, 2020

As per #19513, this PR adds a label to the search results for initial suggestions.

Note: this does not attempt to address this feedback.

Could the URL protocol be dropped?

This will be handled in another PR.

How has this been tested?

  • Unit tests
  • Manual testing:

To test:

  • Add Nav Block
  • See initial search suggestions
  • See "Recently modified" above search results.

Screenshots

Screenshot 2020-01-15 at 11 54 39

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@getdave getdave added Needs Design Feedback Needs general design feedback. Needs Technical Feedback Needs testing from a developer perspective. labels Jan 15, 2020
Copy link
Contributor

@frontdevde frontdevde left a comment

Choose a reason for hiding this comment

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

Tested as per instructions and it worked as expected. Code looks good as well 👍
Props for using the addition of the label to improve the a11y, nice one! 👏

@getdave getdave requested a review from obenland January 15, 2020 14:36
@shaunandrews
Copy link
Contributor

Looks good but I question the use of the word “modified”. I think “Recently updated” was more approachable — modified feels strange in this context.

<div { ...suggestionsListProps } className={ resultsListClasses }>
{ ! isInitialSuggestions ? <VisuallyHidden>{ SearchResultsLabel }</VisuallyHidden> : SearchResultsLabel }

<div { ...suggestionsListProps } className={ resultsListClasses } aria-labelledby={ searchResultsLabelId }>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice job on adding aria-labelledby! 🙌

I think there may be a slight difference on using that on visible vs hidden labels: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/listbox_role

I'm not positive I'm fully understanding the implementation though:

aria-label
A human-readable string value which identifies the listbox. If there's a visible label, then aria-labelledby should be used instead to refer to that label.

aria-labelledby
Identifies the visible element or elements in a space-separated list of element IDs which identify the listbox. If there's no visible label, then aria-label should be used instead to include a label. (Note: "labelled", with two L's, is the correct spelling based on the accessibility API conventions.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch but I don't follow the a11y guidelines here. Do I need to just add aria-label on the element when it's not visible?

@getdave
Copy link
Contributor Author

getdave commented Jan 15, 2020

@shaunandrews I've updated to "updated" 😆. On that basis are you happy to 👍 ?

Copy link
Contributor

@shaunandrews shaunandrews left a comment

Choose a reason for hiding this comment

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

🚢

@getdave getdave merged commit c53b94d into master Jan 15, 2020
@getdave getdave deleted the add/label-to-link-control-initial-suggestions-results branch January 15, 2020 17:06
@ellatrix ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block Needs Design Feedback Needs general design feedback. Needs Technical Feedback Needs testing from a developer perspective.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants