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

feat(accessibility): optionally disable aria-current tag #582

Merged

Conversation

akashdeep-sarin
Copy link
Contributor

Some of the accessibility specifications require removal of aria-current tag in lists.
image

This tag is displayed on the currently focused <ListItem>'s.
image

For this, I am suggesting an ariaConfig prop which may contain a JSON value for tweaking accessibility properties.

Here:

const ariaConfiguration = {disableAriaCurrent : true};
<List ariaConfig={ariaConfiguration }> 

Can be used to stop the population of aria-current = true tag.

TODOs

  • Write tests
  • Update Snapshots

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots:

Before (If applicable):

After:

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 change)
  • Documentation update
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Contributor

@holsted holsted left a comment

Choose a reason for hiding this comment

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

@akashdeep-sarin how many other aria props are there? I like the idea of passing an object for configuration of aria properties but was curious as we have a mostly flat prop structure

Copy link
Contributor

@pauljeter pauljeter left a comment

Choose a reason for hiding this comment

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

You need to update your snapshots and add them to the PR. Tests are failing in CI

yjlo
yjlo previously approved these changes Apr 6, 2020
Copy link
Collaborator

@yjlo yjlo left a comment

Choose a reason for hiding this comment

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

Address the CI issues, other than that, everything else looks good.

@akashdeep-sarin
Copy link
Contributor Author

akashdeep-sarin commented Apr 6, 2020

@andrewholsted - We have more than a dozen of aria-* attributes . link, I thought of passing a JSON object, since the behavior of placement of aria-current tag is handled by the momentum-ui code. In most other cases, the tags are static and dont change their values and/or location in the DOM.
Its just a design decision - can be done otherwise too... by sending a flat props

@akashdeep-sarin
Copy link
Contributor Author

@pauljeter - Yes. Will be updating,

Copy link
Contributor

@bfbiggs bfbiggs left a comment

Choose a reason for hiding this comment

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

@akashdeep-sarin is that thought here that we might have other ariaConfig options?

@akashdeep-sarin
Copy link
Contributor Author

@bfbiggs - Yes... In another usecase, I need (optionally?) specify aria-posinset tags...
I think for such tweaks in ARIA behaviour its better to use a single ariaConfig prop... which can be customized for configuring such behaviour...
Many similar use cases could be possible...

@akashdeep-sarin
Copy link
Contributor Author

Added Test Cases
Updated Snapshots
Pipeline Passing
Added ariaConfig props to Lists and Menus

@pauljeter pauljeter requested a review from aimeex3 April 23, 2020 16:45
@pauljeter pauljeter dismissed their stale review April 23, 2020 16:50

Shapshots have been updated

@holsted holsted merged commit 0165ab6 into momentum-design:master Apr 23, 2020
yjlo pushed a commit to yjlo/momentum-ui that referenced this pull request Jun 24, 2020
…sign#582)

* feat(accessibility): optionally disable aria-current tag

* chore(list): add/update testcases

Co-authored-by: Akashdeep Singh <aksinghs@cisco.com>
yjlo pushed a commit to yjlo/momentum-ui that referenced this pull request Jun 24, 2020
…sign#582)

* feat(accessibility): optionally disable aria-current tag

* chore(list): add/update testcases

Co-authored-by: Akashdeep Singh <aksinghs@cisco.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants