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

attributes in AccessiblePeer should all use setAttributeToElement #895

Closed
jessegreenberg opened this issue Nov 12, 2018 · 3 comments
Closed
Assignees

Comments

@jessegreenberg
Copy link
Contributor

From #893 @zepumph pointed out that it would be better if all attributes used the function that was made for this for consistency.

@jessegreenberg
Copy link
Contributor Author

Took a look at this and I am now questioning the decision. There are three places outside of setATtributeToElement that set an attribute. Two were straight forward, but the third is in AccessiblePeer.setVisible

        for ( var i = 0; i < this.topLevelElements.length; i++ ) {
          var element = this.topLevelElements[ i ];
          if ( visible ) {
            element.removeAttribute( 'hidden' );
          }
          else {
            element.setAttribute( 'hidden', '' );
          }
        }

It is nice to be able to apply hidden to attributes in a loop without having to go through the "name" of the element like AccessiblePeer.CONTAINER_PARENT.

@jessegreenberg
Copy link
Contributor Author

On the other hand, this is exactly where we would want to use the asProperty option so we don't have to do element.setAttribute( 'hidden', '' );

@jessegreenberg
Copy link
Contributor Author

Done in the above commits. For setVisible I added an element option to setAttributeToElement to set the attribute directly to the desired element rather than looking up by name.

I considered replacing this entirely with the elementName option, but that would have required using getElementByName at every usage of setElementToAttribute so I decided against it.

Local tests are passing, closing this issue.

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

No branches or pull requests

1 participant