Skip to content

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