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: add accessibilityLabel to Pagination #438

Merged
merged 2 commits into from
Dec 11, 2018

Conversation

thymikee
Copy link
Contributor

Platforms affected

All

What does this PR do?

Adds optional accessibilityLabel to <Pagination /> component so it's possible to e.g. display which slide we're showing when focusing is on pager.

What testing has been done on this change?

We're using this in one of the projects I'm working on and wanted to back-port the change to upstream :)

Tested features checklist

@bd-arc
Copy link
Contributor

bd-arc commented Dec 11, 2018

Hey @thymikee,

Thanks for the PR!

I've never used the accessibility features of React Native, but I'm a bit concerned about the description of prop accessible:
When a view is an accessibility element, it groups its children into a single selectable component.

Isn't this going to be a problem?

@thymikee
Copy link
Contributor Author

It shouldn't be a problem because we provide an accessibilityLabel which drives this parent element. Also this description relates to voice over, not the actual component structure.

@bd-arc
Copy link
Contributor

bd-arc commented Dec 11, 2018

Ok, thanks for clarifying.

Still, I would prefer that you add a condition like this one: accessible={!!accessibilityLabel}.

@thymikee
Copy link
Contributor Author

Updated :)

@bd-arc
Copy link
Contributor

bd-arc commented Dec 11, 2018

Thanks a lot! Let's merge this ;-)

@bd-arc bd-arc merged commit 85980e5 into meliorence:master Dec 11, 2018
@thymikee thymikee deleted the feat/accessible-pagination branch December 11, 2018 14:55
@thymikee
Copy link
Contributor Author

Thank you as well :)
BTW would be cool to setup some automated tests, type checks and linters, because it feels rough and scary when contributing and it must be burden for you and other maintainers as well. I'm probably not in capacity of tackling this, but would be cool to at least start the effort by creating issues (if there is none already) 🙂.

@bd-arc
Copy link
Contributor

bd-arc commented Dec 11, 2018

I totally agree with you. This is one of the house-keeping things that are required, but unfortunately I haven't been able to contribute as much as I wanted to the repo lately...

Definitely feel free to open an issue for that ;-)

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