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

Add implementation of UIAccessibilityContainer Protocol #32

Merged
merged 2 commits into from
Apr 1, 2014

Conversation

JanenePappas
Copy link
Contributor

Addresses #31, to add accessibility; creates accessible element consisting of the title and description when a new message is shown and notifies the framework to read the message

@JanenePappas
Copy link
Contributor Author

@terryworona You would prefer just the accessible element instance and not the usual pattern of keeping a collection? Certainly can do it that way. Will modify and re-submit.

The accessibility frame is actually irrelevant (found after more testing after your comment). I'll remove it since the user can't tap on it anyway.

@terryworona
Copy link
Owner

I'm not really saying to do it one way or another, just curious why it's a collection if we only really have a single element to keep track of; I guess a collection scales better for (potential) future changes.

/**
* Enable Accessibility
*/
@property (nonatomic, strong) NSMutableArray *accessibleElements;

Choose a reason for hiding this comment

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

If this does remain a collection, this property doesn't need to be publicly exposed regardless.

Choose a reason for hiding this comment

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

(And definitely doesn't need to be mutable.)

…s irrelevant - accessibility will trigger reading when shown then disappear
@JanenePappas
Copy link
Contributor Author

I've submitted an updated push for keeping a single element. I would agree that a collection is definitely more scalable, however.

@JanenePappas JanenePappas changed the title Add implementation of UIAccessibilityProtocol Add implementation of UIAccessibilityContainer Protocol Mar 31, 2014
@cdzombak
Copy link

👍

@terryworona
Copy link
Owner

This looks good. I'll pull it in later tonight or tomorrow. Thanks for the contrib.

@JanenePappas
Copy link
Contributor Author

Great, thanks Terry. Happy to add something to the project.

Janene

On Mar 31, 2014, at 8:04 PM, terryworona notifications@github.com wrote:

This looks good. I'll pull it in later tonight or tomorrow. Thanks for the contrib.


Reply to this email directly or view it on GitHub.

terryworona added a commit that referenced this pull request Apr 1, 2014
Add implementation of UIAccessibilityContainer Protocol
@terryworona terryworona merged commit 120753d into terryworona:master Apr 1, 2014
@terryworona
Copy link
Owner

I pulled it in today and made some cleanup changes.

I'm a bit of a novice when it comes to accessibility; but I tested it on device and the voice doesn't exactly work (the message is cut off when I tap the button to present it). Am I missing something?

@JanenePappas
Copy link
Contributor Author

The changes are structured to initiate the voice-over immediately when the banner is presented. It does not cut out unless the user taps on it, as tapping on the banner is set to dismiss it in itemSelected. If you do nothing, does it complete reading of the message?

@terryworona
Copy link
Owner

The voice over begins, but is then cut off by another voice over describing the button that was pressed to show the message; this is all within the demo app.

You mind checking out the demo project in master to see if you can reproduce?

@JanenePappas
Copy link
Contributor Author

Will do.

@terryworona
Copy link
Owner

Sweet. Go ahead and pull now, I just checked in a last minute change! Thanks!

@JanenePappas
Copy link
Contributor Author

It appears the demo's view controller is affecting accessibility. I've put your version of TWMessageBarManager in my app and it works correctly. Researching more.

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.

3 participants