Skip to content

Accessibility for action buttons. #308

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dlombardi
Copy link

@dlombardi dlombardi commented Feb 15, 2019

Announces action button items when action button is active and the platform is ios. Focuses on top button when action button is active.

…action button is active and the platform is ios. Focuses on top button when action button is active.
@dlombardi dlombardi changed the title Accessibility for action buttons. Announces action button items when… Accessibility for action buttons. Feb 15, 2019
Copy link
Contributor

@arelstone arelstone left a comment

Choose a reason for hiding this comment

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

Love this. Thanks a lot. I am in need of this for a project too ❤️ 😍
I'm added a few comments on improvements that I would make

}
const reactTag = findNodeHandle(ref);

Platform.OS === 'android' ? UIManager.sendAccessibilityEvent(
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use the isAndroid-method witch is imported on line line 19 instead of this


const focusOnView = (ref) => {
if (!ref) {
console.warn('ref is null');
Copy link
Contributor

Choose a reason for hiding this comment

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

If this console.warn should stay, witch I think it could. I think that the error message should be better then this.

this.state = {
resetToken: props.resetToken,
active: props.active
active: props.active,
Copy link
Contributor

Choose a reason for hiding this comment

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

Yea I'm on #teamtrailingcomma ❤️

} = this.props;

if (nextProps.resetToken !== resetToken) {
if (nextProps.active === false && active === true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick:
The === true and === false is redundant.

Suggested change
if (nextProps.active === false && active === true) {
if (!nextProps.active && active) {

Also, personally i'm not a fan of nested if statements

return `${acc} ${actionButton.props.accessibilityLabel}, `;
}, announceActionsLabel);

if (actionButtons.length && Platform.OS === 'ios') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick:
Maybe you should add an isIOS-method to shared.js, that checks the that Platform.OS === 'ios', and use is here

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.

2 participants