Donburi/don 335 UI tests flights place selector additional #1944
Conversation
| @@ -86,12 +86,13 @@ public struct BPKAppSearchModal: View { | |||
| .accessibilityHidden(false) | |||
| .accessibilityRemoveTraits(.isImage) | |||
| .accessibilityAddTraits(.isButton) | |||
| .accessibilityLabel(closeAccessibilityLabel) | |||
| .accessibilityLabel(closeAccessibilityLabel.lowercased()) | |||
There was a problem hiding this comment.
I don't think we should lowercase the accessibility label, is there any specific use-case where you see this go wrong?
There was a problem hiding this comment.
we pretty sure need to remove that
I did it because UITests didn't see any element with non-lowercased localisable string in it
the issue was 100% reproducible...until I restart my machine (which I did just in case)
so, now I don't see any problems with that
| BPKText(title, style: .heading5) | ||
| .padding(.vertical, .sm) | ||
| .accessibilityAddTraits(.isHeader) | ||
| .accessibilityLabel(title) |
There was a problem hiding this comment.
Applying the label should not make a difference, as the BPKText already has the same visible label, did you hit any issues with this?
There was a problem hiding this comment.
yes, you're right and this line was removed from the initial PR, this change here is outdated
| .accessibilityElement(children: .ignore) | ||
| .accessibilityLabel(accessibilityLabel) | ||
| .accessibilityAddTraits(.isButton) | ||
| .opacity(text.isEmpty ? 0.0 : 1.0) |
There was a problem hiding this comment.
Should we make the if case check if text.isEmpty and not add the clear button?
I'm worried that setting opacity might not be foolproof for the screen reader?
What happens if the button is activated and it is then set to 0, does the focus linger on it?
There was a problem hiding this comment.
Actually, it's working great with the accessibility reader. But only if it's after accessibility modifiers. Make it inside if statement is not recommended due to performance possible issues.
yurareutskiy
force-pushed
the
donburi/DON-335_ui_tests_flights_place_selector_additional
branch
from
f698e55 to
e23c04f
Compare
Remember to include the following changes:
README.mdBackpack.hheader fileIf you are curious about how we review, please read through the code review guidelines