fix(EmptyState): Missing accessibility ids on the Empty State - SMARTWIFI-3093 #148
Conversation
| @@ -17,7 +17,7 @@ import UIKit | |||
| /// the spaces between the texts of a letter. When a text is not configured, her vertical spacing is also ignored. | |||
| @dynamicMemberLookup | |||
| class StackViewContentItem<Element: UIView>: UIStackView { | |||
| private var item: Element | |||
| var item: Element | |||
There was a problem hiding this comment.
I had to make this internal... dynamicMemberLookup is sometimes a pain, when the object has a property with a name, it won't use the dynamic lookup... for example when creating a view like let label = StackViewContentItem<UILabel>(topSpacing: 0) and accessing label.accessibilityIdentifier, label.isHidden, etc... as the StackView already has those properties you will be accessing those and not the item's property...
| @@ -24,7 +24,7 @@ DEVICES_USED_FOR_TESTING = ["iPhone 13"] | |||
| platform :ios do | |||
|
|
|||
| before_all do | |||
| xcversion(version: "13") | |||
| xcversion(version: "13") if is_ci | |||
There was a problem hiding this comment.
failing to execute locally if you don't have an xcode named "Xcode_13.app", I removed this constraint so it is executed only on CI
pbartolome
requested review from
a team,
jmbrocal,
jmpg93 and
alexanegon
and removed request for
a team and
jmbrocal
pbartolome
changed the title
| let titleAccesibilityIdentifier: String? | ||
| let descriptionAccesibilityIdentifier: String? | ||
| let assetAccesibilityIdentifier: String? |
There was a problem hiding this comment.
Hmm I'm not sure about this one:
- Here we are using the init to provide the identifiers (no other component does that)
- In
HighlightedCard(and others) we are exposing computed properties to get and set the identifiers. - In the
Calloutwe are exposing the inner components.
I would go with the same approach as HighlightedCard so at least almost every component behaves in the same way.
WDYT?
There was a problem hiding this comment.
ok, I thought about that it was a pain to then propagate those attributes to each subelement, I will change it and see how it looks... and what about the EmptyStateButton? there I'm using the init as well
| @@ -11,24 +11,30 @@ import Foundation | |||
| public struct EmptyStateButton { | |||
| public let title: String | |||
| public let loadingTitle: String? | |||
| public let accesibilityIdentifier: String? | |||
There was a problem hiding this comment.
Hmm feels weird to have this here.
Would be nice to be able to do the same as the title/description.
vc.emptyState.titleAccessibilityIdentifier = "empty_state_title_id"
vc.emptyState.descriptionAccessibilityIdentifier = "empty_state_card_id"
vc.emptyState.assetAccessibilityIdentifier = "empty_state_card_id"
vc.emptyState.secondaryButtonAccessibilityIdentifier = "empty_state_card_id"
vc.emptyState.primaryButtonAccessibilityIdentifier = "empty_state_card_id"
vc.emptyState.linkAccessibilityIdentifier = "empty_state_card_id"Any particular reason for not doing it this way too? Maybe the way the actions are configured?
There was a problem hiding this comment.
no reason, the same as I did initially, add indentifiers to the init, it seemed easier to then pass that configuration to the actual element, I changed it to follow the same approach as the other elements
|
🎉 This PR is included in version 14.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Add properties to the EmptyStateConfiguration to allow changing accessibility identifiers of its elements