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

IOS-10414 Add Skeleton components in SwiftUI and UIKit #388

Merged
merged 8 commits into from
Jul 23, 2024

Conversation

alejandroruizponce
Copy link
Contributor

@alejandroruizponce alejandroruizponce commented Jul 22, 2024

🎟️ Jira ticket

https://jira.tid.es/browse/IOS-10414

πŸ₯… What's the goal?

Implement the native Skeletons components designed by Mistica in the following spec:
https://www.figma.com/design/w7E0mmB92eio0zHw7h9iS2/%F0%9F%94%B8-Skeletons-Specs?node-id=986-1161&t=SJOFSJMtw5Mmpp22-0

🚧 How do we do it?

The following changes have been made:

  • Two new views have been added. One called Skeleton for SwiftUI and one called SkeletonView for UIKit.
  • Both views are driven by Figma specs and their behavior is similar.
  • They have also been added to the Mistica Catalog for testing.
  • Tests have been added for both.
SwiftUI UIKit
Β Screenshot 2023-10-03 at 15 27 29Β  Screenshot 2023-10-03 at 15 30 48

πŸ§ͺ How can I verify this?

A version of the MisticaCatalog has been generated to test and visualize the Skeletons.

πŸ‘ AppCenter build

(Jul 22, 2024 at 17:56)
Screenshot 2024-07-22 at 18 02 40

Copy link
Contributor

@DavidMarinCalleja DavidMarinCalleja left a comment

Choose a reason for hiding this comment

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

Comment on lines +11 to +17
public enum SkeletonType {
case line(width: CGFloat)
case text(numberOfLines: Int = 3)
case circle(size: CGFloat)
case row
case rectangle(size: CGSize, isRounded: Bool)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe I would have a LineSkeletonView, TextSkeletonView,... to avoid executing a switch eeeeverytime.
If you know the type you need, you are able to call the right View :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm, I don't necessarily agree. IMO for such simple views that share common attributes between them and that are even reused in certain types it makes more sense to have them centralized. It brings consistency.

In the case of a higher level of complexity in the views and contain significant differences between them if I could agree to separate them.

But right now I see no compelling reasons, neither of testability nor of readability. WDTY?

}

public var body: some View {
switch type {
Copy link
Contributor

Choose a reason for hiding this comment

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

same. we are executing this switch every time the view is rendered. Why don't you create several views?


final class SkeletonTests: XCTestCase {
override class func setUp() {
isRecording = false
Copy link
Contributor

Choose a reason for hiding this comment

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

what about the snapshots? I couldn't find them...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was waiting for the approval to launch the GH action of the record. But maybe it helps to see the snapshots already to review.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it could help, thanks!

override class func setUp() {
super.setUp()

isRecording = false
Copy link
Contributor

Choose a reason for hiding this comment

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

what about the snapshots? I couldn't find them...

public struct Skeleton: View {
enum Constants {
static var lineHeight = 8.0
static var radius = 8.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Radius should use tokens. Was our mistake because the specs were not defined well

https://github.com/Telefonica/mistica-design/blob/689b9891d05356107da646f969224c826cbcaeb5/tokens/movistar.json#L1355

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I didn't remember it either although I didn't see it in the specs. Fixed!

@alejandroruizponce
Copy link
Contributor Author

alejandroruizponce commented Jul 23, 2024

Record screenshots on PR comment: succeeded βœ…
https://github.com/Telefonica/mistica-ios/actions/runs/10057430980

Copy link
Contributor

@yceballost yceballost left a comment

Choose a reason for hiding this comment

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

nice job! 🀩

Copy link

github-actions bot commented Jul 23, 2024

Screenshot tests report

βœ”οΈ All passing

@alejandroruizponce
Copy link
Contributor Author

alejandroruizponce commented Jul 23, 2024

Record screenshots on PR comment: succeeded βœ…
https://github.com/Telefonica/mistica-ios/actions/runs/10058638033

@alejandroruizponce alejandroruizponce merged commit e99d295 into main Jul 23, 2024
1 check passed
@alejandroruizponce alejandroruizponce deleted the IOS-10414-Add-skeletons-component branch July 23, 2024 15:16
@tuentisre
Copy link
Collaborator

πŸŽ‰ This PR is included in version 31.3.0 πŸŽ‰

The release is available on GitHub release

Your semantic-release bot πŸ“¦πŸš€

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants