fix(EmptyState): Empty state buttons handler IOS-7012 #137
Conversation
alexanegon
requested review from
DavidMarinCalleja and
jmbrocal
and removed request for
a team
alexanegon
dismissed stale reviews from DavidMarinCalleja and jmbrocal
via
aa031a8
alexanegon
force-pushed
the
bug/IOS-7012-empty-state-buttons-handler
branch
from
18571bc to
aa031a8
Compare
| public struct EmptyStateIcon: Equatable { | ||
| public let image: UIImage | ||
| public let tint: UIColor? | ||
|
|
||
| public init(image: UIImage, | ||
| tint: UIColor? = nil) { | ||
| self.image = image | ||
| self.tint = tint | ||
| } | ||
| } |
There was a problem hiding this comment.
I do not like this approach because IMHO it is complicating the API of the component.
From iOS 7 you can tint a UIImage with withRenderingMode(.alwaysTemplate) , then the user of the can use a tinted image for this component like
let tintedImage = UIImage(named: "myImageName").withRenderingMode(.alwaysTemplate)
let component = EmptyState(type: .default(.icon(tintedImage))), title: "foooo")Other point against the proposed domain is that you should use this new type for every UIImage of EmptyStateDefaultAsset ...
There was a problem hiding this comment.
I decided this way because in versions prior to iOS 13 we can not apply the tintcolor directly to the image, it must be applied in the ImageView.
There was a problem hiding this comment.
Ups right, and we only support iOS 12 in Mistica....
Then we should use the EmptyStateIcon on every UIimage of EmptyStateDefaultAsset or we can expose a var in EmptyState for tinting the asset, something like: assetTintColor.
There was a problem hiding this comment.
mmm the thing is that EmptyStateIcon is just for Icons the other assets are images that I think that is not expected to be tinted.
I expose the property to tint the color
alexanegon
force-pushed
the
bug/IOS-7012-empty-state-buttons-handler
branch
from
786f739 to
88b6b17
Compare
alexanegon
force-pushed
the
bug/IOS-7012-empty-state-buttons-handler
branch
from
88b6b17 to
7310824
Compare
| @@ -10,7 +10,7 @@ import Foundation | |||
| import UIKit | |||
|
|
|||
| public struct EmptyStateConfiguration { | |||
| static let empty = EmptyStateConfiguration(type: .default(.icon(UIImage(color: .success))), title: "Basic configuration", description: "This is a basic configuration for the empty state", actions: nil) | |||
| static let empty = EmptyStateConfiguration(type: .default(.icon(EmptyStateIcon(image: UIImage(color: .success)))), title: "Basic configuration", description: "This is a basic configuration for the empty state", actions: nil) | |||
There was a problem hiding this comment.
no related with the bug but, why do we need this empty configuration? I did not found this requirement in the spec of the component.
There was a problem hiding this comment.
It is just in case the developer did not fill the configuration
| @@ -43,7 +43,7 @@ public struct EmptyStateConfiguration { | |||
| let description: String? | |||
| let actions: EmptyStateActions? | |||
|
|
|||
| public init(type: EmptyStateType = .default(.icon(UIImage())), title: String, description: String?, actions: EmptyStateActions? = nil) { | |||
| public init(type: EmptyStateType = .default(.icon(EmptyStateIcon(image: UIImage()))), title: String, description: String?, actions: EmptyStateActions? = nil) { | |||
There was a problem hiding this comment.
The default value of the type parameter is pretty ugly, we are passing an empty image which will result in a imageview without any image.
The spec says that the asset is mandatory for this component: zeplin link. I would removed the default value for this parameter.
I have doubts about the optional of description parameter because reviewing the spec I did not found an example of this component without subtitle. Should this parameter be non optional?
There was a problem hiding this comment.
I agree with Jose, if the asset is mandatory I would remove that default value. about the description I would ask the design team if this parameter is also mandatory.
There was a problem hiding this comment.
You are truly right! I deleted the default value
alexanegon
force-pushed
the
bug/IOS-7012-empty-state-buttons-handler
branch
from
5bfeb09 to
b4a69af
Compare
# [12.0.0](v11.4.0...v12.0.0) (2021-09-16) ### Bug Fixes * **EmptyState:** Controlador de estado vacío de botones IOS-7012 ([#137](#137)) ([d14a4fd](d14a4fd)) * refactor(SegmentSelector) Rename SegmentSelector to Filter (#136) ([6830ea9](6830ea9)), closes [#136](#136) ### BREAKING CHANGES * SegmentSelector as been renamed to Filter.
|
🎉 This PR is included in version 12.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎟️ Jira ticket
IOS-7012 Empty state buttons handler not working
🥅 What's the goal?
Assign the handler defined by the developer to the desired buttons of the component
🚧 How do we do it
We did it by assigning the handler defined outside of the component to the component vars defined for that
🧪 How can I test this?
Downloading the catalog and inside the Empty state component tap any button.
https://install.appcenter.ms/orgs/tuenti-organization/apps/mistica-ios/distribution_groups/public
🏑 AppCenter build
Last version
📱 Demo
Grabacion.de.pantalla.2021-09-15.a.las.17.45.54.mov