Skip to content
This repository has been archived by the owner on Sep 20, 2023. It is now read-only.

Improve accessibility of more options button and notification cell #119

Merged
merged 13 commits into from Jul 19, 2017

Conversation

BasThomas
Copy link
Collaborator

Ref #118

This PR:

  • Makes the More Options button in the issue overview say "More Options" instead of "bullets-hollow"
  • Makes the notification cell accessible as a whole, as well as mentioning its type (issue, pr or commit)

BasThomas and others added 7 commits July 11, 2017 19:40
…abels

This makes the cell an accessibility element (eg. Review GitHub Access under Settings) and mention that A: the cell is a button and B: its label is "Review GitHub Access"

Before this change, the label was an element and did not mention it was also a button
@BasThomas BasThomas mentioned this pull request Jul 12, 2017
29 tasks
@@ -98,6 +100,10 @@ final class NotificationCell: SwipeSelectableCell {
textLabel.attributedText = viewModel.title.attributedText
dateLabel.setText(date: viewModel.date)
reasonImageView.image = viewModel.type.icon?.withRenderingMode(.alwaysTemplate)
accessibilityLabel = contentView.subviews
.flatMap { $0.accessibilityLabel }
.reduce("", { $0 + ".\n" + $1 })
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the ./n-s makes VoiceOver read these as sentences instead of just one, long sentence, making the different labels / elements separated.

Copy link
Member

Choose a reason for hiding this comment

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

Same note about moving this to override?

Copy link
Member

Choose a reason for hiding this comment

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

What about this one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See the comment on the other one:

That would be a little restricting though - we can't access the viewModel in the NotificationCell, which I used to provide the type (Issue / PR / commit).

@@ -47,6 +57,9 @@ class SelectableCell: UICollectionViewCell {

override func layoutSubviews() {
super.layoutSubviews()
accessibilityLabel = contentView.subviews
.flatMap { $0.accessibilityLabel }
.reduce("", { "\($0 ?? "").\n\($1)" })
Copy link
Member

Choose a reason for hiding this comment

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

Mind moving this to an override of accessibilityLabel? Then it only executes when necessary instead of every layout pass.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would be a little restricting though - we can't access the viewModel in the NotificationCell, which I used to provide the type (Issue / PR / commit).

@@ -98,6 +100,10 @@ final class NotificationCell: SwipeSelectableCell {
textLabel.attributedText = viewModel.title.attributedText
dateLabel.setText(date: viewModel.date)
reasonImageView.image = viewModel.type.icon?.withRenderingMode(.alwaysTemplate)
accessibilityLabel = contentView.subviews
.flatMap { $0.accessibilityLabel }
.reduce("", { $0 + ".\n" + $1 })
Copy link
Member

Choose a reason for hiding this comment

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

Same note about moving this to override?

@rnystrom
Copy link
Member

SO GOOD, I love it

@rnystrom rnystrom merged commit 6859fdb into master Jul 19, 2017
rnystrom added a commit that referenced this pull request Jul 19, 2017
@BasThomas BasThomas deleted the improve-accessibility branch July 24, 2017 14:25
@BasThomas BasThomas changed the title Improve accessibility Improve accessibility of more options button and notification cell Oct 26, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants