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

Should Accessibility.js be able to update peers directly? #773

Closed
zepumph opened this issue Apr 22, 2018 · 4 comments
Closed

Should Accessibility.js be able to update peers directly? #773

zepumph opened this issue Apr 22, 2018 · 4 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Apr 22, 2018

At a high level, I have always thought that there was modularity in function between AccessibleInstance and AccessiblePeer, such that

Node ---> AccessibleInstance ---> AccessiblePeer

So a node can have n AccessibleInstances and each AccessibleInstance has 1 AccessiblePeer. From this thought process, I would expect that AccessiblePeer can only be accessed from the instance, for modularity's sake.

In Accessibility.js I see updateAccessiblePeers which directly through instances updates the accessiblePeers. To me this seems like it breaks the modularity.

I would expect that Node (Accessibility.js) would only interact with its AccessibleInstances, I think it may be nicer if AccessibleInstance it the only thing that updates it accessible peer. @jessegreenberg what do you think? Tagging @jonathanolson for comment too. I can't think of any specific reason why it is a large benefit, but I do feel like the current implementation is a code smell.

@zepumph zepumph changed the title Should Accessibility.js be able to update peers directly. Should Accessibility.js be able to update peers directly? Apr 22, 2018
@jonathanolson
Copy link
Contributor

I'm not too concerned, since this is scenery-internal. It looks like shared behavior that factors code out, basically taking the two steps Node => AccessibleInstance and AccessibleInstance => AccessiblePeer into one step.

@jessegreenberg
Copy link
Contributor

@zepumph here is one idea for an alternative.

        Accessibility.updateAccessibleInstances: function( callback ) {
          for ( var i = 0; i < this._accessibleInstances.length; i++ ) {
            this._accessibleInstances[ i ].updatePeer( callback );
          }
        },
        AccessibleInstance.updateAccessiblePeer: function( callback ) {
          this.peer.updatePeer( callback );
        },

Could also consider something different like specific functions for each callback passed to updateAccessiblePeers.

@zepumph
Copy link
Member Author

zepumph commented May 25, 2018

Does this seem like the right way to go? Is it even worth it? I see 14 usages of updateAccessiblePeers and only one of getAccessibleInstances. Maybe I just didn't really understand the relationship.

I'm happy to implement this if you think it is worth it. I don't think it would be too hard.

@zepumph
Copy link
Member Author

zepumph commented Jul 4, 2018

As part of #814 we are doing a lot of work related to this. Right now we are going to tell the peer to update directly from the Node, but the Peer is actually going to do the updating, so things are more modular. Closing

@zepumph zepumph closed this as completed Jul 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants