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

[BPKBarChart] ACC-455/ACC-456 Resolve Accessibility defects #1964

Merged
merged 7 commits into from
May 22, 2024

Conversation

gert-janvercauteren
Copy link
Contributor

@gert-janvercauteren gert-janvercauteren commented May 22, 2024

BPKBarChart

  • Hide the key items (price/no price) as they add clutter
  • Make month titles heading
  • Fix accessibility label and value for each bar
Before After
Video before Video after

Remember to include the following changes:

If you are curious about how we review, please read through the code review guidelines

@gert-janvercauteren gert-janvercauteren self-assigned this May 22, 2024
@@ -26,7 +26,6 @@ internal final class BPKBarChartCollectionViewCell: UICollectionViewCell {
lazy public var barChartBar: BPKBarChartBar = {
let view: BPKBarChartBar = BPKBarChartBar()
view.translatesAutoresizingMaskIntoConstraints = false
view.isUserInteractionEnabled = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would add 'dimmed' to the barchart read out for each bar

@@ -20,10 +20,10 @@ import UIKit

@objcMembers
@objc(BPKBarChartBar)
public final class BPKBarChartBar: UIControl {
public final class BPKBarChartBar: UIView {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make the bar a simple UIView, we only used UIControl to facilitate isSelected as a property

Copy link
Contributor

Choose a reason for hiding this comment

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

Any cons in leaving this a UIControl? Given is an intractable element and UIControl inherits from UIView, seems alright to leave it like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that we then need to set userInteractionEnabled = false on it, to allow selection of the cell.
There was no point in having it as a UIControl, if we do and disable interaction it will announce as disabled :/

@gert-janvercauteren gert-janvercauteren merged commit 34fd203 into main May 22, 2024
16 checks passed
@gert-janvercauteren gert-janvercauteren deleted the a11y/ACC-455_barchart-fixes branch May 22, 2024 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility patch Patch production bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants