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.

Sign up for GitHub

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

fix(EmptyState): Missing accessibility ids on the Empty State - SMARTWIFI-3093 #148

Merged
merged 5 commits into from
Dec 3, 2021
Merged

fix(EmptyState): Missing accessibility ids on the Empty State - SMARTWIFI-3093 #148

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
12470c2
SMARTWIFI-3093 Missing accessibility ids on the Empty State
pbartolome Dec 2, 2021
8cfc998
Run swiftformat
pbartolome Dec 2, 2021
fa1737e
SMARTWIFI-3093 Move EmptyState accesibility identifiers outside the init
pbartolome Dec 2, 2021
d1438f2
SMARTWIFI-3093 Move EmptyState accesibility identifiers outside the init
pbartolome Dec 3, 2021
f771b61
Run swiftformat
pbartolome Dec 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion Mistica/Source/Components/EmptyState/EmptyStateConfiguration.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,25 @@ public struct EmptyStateConfiguration {
let type: EmptyStateType
let title: String
let description: String?
let titleAccesibilityIdentifier: String?
let descriptionAccesibilityIdentifier: String?
let assetAccesibilityIdentifier: String?
Copy link
Contributor

Choose a reason for hiding this comment

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

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 Callout we 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?

Copy link
Contributor Author

@pbartolome pbartolome Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

let actions: EmptyStateActions?

public init(type: EmptyStateType, title: String, description: String?, actions: EmptyStateActions? = nil) {
public init(type: EmptyStateType,
title: String,
description: String?,
titleAccesibilityIdentifier: String? = nil,
descriptionAccesibilityIdentifier: String? = nil,
assetAccesibilityIdentifier: String? = nil,
actions: EmptyStateActions? = nil) {
self.type = type
self.title = title
self.description = description
self.titleAccesibilityIdentifier = titleAccesibilityIdentifier
self.descriptionAccesibilityIdentifier = descriptionAccesibilityIdentifier
self.assetAccesibilityIdentifier = assetAccesibilityIdentifier

self.actions = actions
}

Expand Down
2 changes: 2 additions & 0 deletions Mistica/Source/Components/EmptyState/Internals/EmptyStateButtons.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ private extension EmptyStateButtons {
button.title = configButton.title
button.loadingTitle = configButton.loadingTitle
button.isSmall = isCard
button.accessibilityIdentifier = configButton.accesibilityIdentifier
if button.superview == nil {
addArrangedSubview(button)
}
Expand All @@ -97,6 +98,7 @@ private extension EmptyStateButtons {
link.title = linkButton.title
link.contentMode = .left
link.isSmall = isCard
link.accessibilityIdentifier = linkButton.accesibilityIdentifier

if link.superview == nil {
addArrangedSubview(link)
Expand Down
1 change: 1 addition & 0 deletions Mistica/Source/Components/EmptyState/Internals/EmptyStateContentBase.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ private extension EmptyStateContentBase {
}
}
iconContainerView.backgroundColor = .clear
iconImage.accessibilityIdentifier = configuration.assetAccesibilityIdentifier
insertArrangedSubview(iconContainerView, at: 0)
}

Expand Down
2 changes: 2 additions & 0 deletions Mistica/Source/Components/EmptyState/Internals/EmptyStateMessagesContent.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ extension EmptyStateMessagesContent {
func configure(withConfiguration configuration: EmptyStateConfiguration) {
title = configuration.title
descriptionTitle = configuration.description
titleLabel.item.accessibilityIdentifier = configuration.titleAccesibilityIdentifier
descriptionLabel.item.accessibilityIdentifier = configuration.descriptionAccesibilityIdentifier
let isCard: Bool = configuration.isInCard()
let titleSpacing: CGFloat = isCard ? Constants.titleTopSpacingCard : Constants.titleTopSpacingDefault
titleTopSpacing = titleSpacing
Expand Down
6 changes: 6 additions & 0 deletions Mistica/Source/Components/EmptyState/Model/EmptyStateButton.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,30 @@ import Foundation
public struct EmptyStateButton {
public let title: String
public let loadingTitle: String?
public let accesibilityIdentifier: String?
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the changes!

public let tapHandler: (() -> Void)?

public init(title: String,
loadingTitle: String?,
accesibilityIdentifier: String? = nil,
tapHandler: (() -> Void)?) {
self.title = title
self.loadingTitle = loadingTitle
self.accesibilityIdentifier = accesibilityIdentifier
self.tapHandler = tapHandler
}
}

public struct EmptyStateLinkButton {
public let title: String
public let accesibilityIdentifier: String?
public let tapHandler: (() -> Void)?

public init(title: String,
accesibilityIdentifier: String? = nil,
tapHandler: (() -> Void)?) {
self.title = title
self.accesibilityIdentifier = accesibilityIdentifier
self.tapHandler = tapHandler
}
}
2 changes: 1 addition & 1 deletion Mistica/Source/Utils/Views/StackViewContentItem.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

@pbartolome pbartolome Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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...


var topSpacing: CGFloat {
didSet {
Expand Down
34 changes: 25 additions & 9 deletions MisticaCatalog/Source/ComponentsCatalogs/UICatalogEmptyStateViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,21 +204,21 @@ extension UICatalogEmptyStateViewController: UITableViewDataSource, UITableViewD
}
switch buttonsCell.segmentedControl.selectedSegmentIndex {
case 0:
actions = .primary(EmptyStateButton(title: "Button small", loadingTitle: nil, tapHandler: handler))
actions = .primary(EmptyStateButton(title: "Button small", loadingTitle: nil, accesibilityIdentifier: "button_small_id", tapHandler: handler))
case 1:
actions = .primaryAndLink(
primary: EmptyStateButton(title: "Button small", loadingTitle: nil, tapHandler: handler),
link: EmptyStateLinkButton(title: "Link", tapHandler: handler)
primary: EmptyStateButton(title: "Button small", loadingTitle: nil, accesibilityIdentifier: "button_small_id", tapHandler: handler),
link: EmptyStateLinkButton(title: "Link", accesibilityIdentifier: "link_id", tapHandler: handler)
)
case 2:
actions = .secondary(EmptyStateButton(title: "Secondary", loadingTitle: nil, tapHandler: handler))
actions = .secondary(EmptyStateButton(title: "Secondary", loadingTitle: nil, accesibilityIdentifier: "secondary_id", tapHandler: handler))
case 3:
actions = .secondaryAndLink(
secondary: EmptyStateButton(title: "Secondary", loadingTitle: nil, tapHandler: handler),
link: EmptyStateLinkButton(title: "Link", tapHandler: handler)
secondary: EmptyStateButton(title: "Secondary", loadingTitle: nil, accesibilityIdentifier: "secondary_id", tapHandler: handler),
link: EmptyStateLinkButton(title: "Link", accesibilityIdentifier: "link_id", tapHandler: handler)
)
case 4:
actions = .link(EmptyStateLinkButton(title: "Link", tapHandler: handler))
actions = .link(EmptyStateLinkButton(title: "Link", accesibilityIdentifier: "link_id", tapHandler: handler))
case 5:
actions = .empty
default:
Expand All @@ -230,7 +230,15 @@ extension UICatalogEmptyStateViewController: UITableViewDataSource, UITableViewD
if isCard {
let image = UIImage(color: .success)
let asset = EmptyStateConfiguration.EmptyStateCardAsset.icon(image)
configuration = EmptyStateConfiguration(type: .card(asset), title: emptyStateTitle, description: emptyStateMessage, actions: actions)
configuration = EmptyStateConfiguration(
type: .card(asset),
title: emptyStateTitle,
description: emptyStateMessage,
titleAccesibilityIdentifier: "empty_state_card_title_id",
descriptionAccesibilityIdentifier: "empty_state_card_description_id",
assetAccesibilityIdentifier: "empty_state_card_asset_id",
actions: actions
)
} else {
let imageDefault = UIImage(color: .success)
let asset: EmptyStateConfiguration.EmptyStateDefaultAsset
Expand All @@ -245,7 +253,15 @@ extension UICatalogEmptyStateViewController: UITableViewDataSource, UITableViewD
default:
fatalError("Case not implemented")
}
configuration = EmptyStateConfiguration(type: .default(asset), title: emptyStateTitle, description: emptyStateMessage, actions: actions)
configuration = EmptyStateConfiguration(
type: .default(asset),
title: emptyStateTitle,
description: emptyStateMessage,
titleAccesibilityIdentifier: "empty_state_title_id",
descriptionAccesibilityIdentifier: "empty_state_description_id",
assetAccesibilityIdentifier: "empty_state_asset_id",
actions: actions
)
}
let vc = EmptyStateViewSampleViewController()
vc.emptyState.contentConfiguration = configuration
Expand Down
3 changes: 2 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,9 @@ By default mistica uses the system font, but if you need to use a custom font (T
* [Checkbox](./Mistica/Source/Components/Checkbox/)
* [Controls](./Mistica/Source/Components/Controls/)
* [Crouton](./Mistica/Source/Components/Crouton/)
* [EmptyState](./Mistica/Source/Components/EmptyState/)
* [Feedbacks](./Mistica/Source/Components/Feedback/)
* [Filter](./Mistica/Source/Components/Filter/)
* [Form](./Mistica/Source/Components/Form/)
* [Header](./Mistica/Source/Components/Header/)
* [InputFields](./Mistica/Source/Components/InputField/)
Expand All @@ -128,7 +130,6 @@ By default mistica uses the system font, but if you need to use a custom font (T
* [RadioButton](./Mistica/Source/Components/RadioButton/)
* [ScrollContentIndicator](./Mistica/Source/Components/ScrollContentIndicator/)
* [SectionTitle](./Mistica/Source/Components/SectionTitle/)
* [Filter](./Mistica/Source/Components/Filter/)
* [Stepper](./Mistica/Source/Components/Stepper/)
* [Switch](./Mistica/Source/Components/Switch/)
* [Tabs](./Mistica/Source/Components/Tabs)
Expand Down
2 changes: 1 addition & 1 deletion fastlane/Fastfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

@pbartolome pbartolome Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

end

desc "Setup the external dependencies and stuff needed to have a full working environment experience"
Expand Down