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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SwiftUI] Add TextStyleLabel #55 #56

Conversation

virginiapujols-yml
Copy link
Contributor

Introduction

Create TextStyleLabel component.

Purpose

Allow TypographyLabel with a single line to be used within SwiftUI. Issue #55

Scope

Added new file TextStyleLabel.swift and updated the README.md

Discussion

Unit Tests: Apple doesn鈥檛 provide any testing framework for SwiftUI yet or a way to have UI test within a Swift Package. Since the solution to include unit tests for this component is not straightforward, I'll create a separate PR for it.

馃幀 Video

Apologies for the bad quality (10MB limit issue):

test-video-3.mov

Copy link
Contributor

@mpospese mpospese left a comment

Choose a reason for hiding this comment

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

Great job with the README! Just need to update comments and mark a few things internal.

Sources/YMatterType/Elements/TextStyleLabel.swift Outdated Show resolved Hide resolved
Sources/YMatterType/Elements/TextStyleLabel.swift Outdated Show resolved Hide resolved
Sources/YMatterType/Elements/TextStyleLabel.swift Outdated Show resolved Hide resolved
Sources/YMatterType/Elements/TextStyleLabel.swift Outdated Show resolved Hide resolved
Sources/YMatterType/Elements/TextStyleLabel.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@mpospese mpospese left a comment

Choose a reason for hiding this comment

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

Looking great Virginia, just a few more things.

let expectedText = Constants.helloWorldText

let label = TypographyLabel(typography: expectedTypography)
let labelConfig = { (label: TypographyLabel) in
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of a local variable, this should be a config passed to the TextStyleLabel initializer.

1st create the sut.
2nd create the label
3rd assert that label.lineBreakMode is not correct
4th call sut.configurationTextStyleLabel(label)
5th assert that label.lineBreakMode was updated.

That proves that the block we passed to the initializer is in fact working.

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 are absolutely right, thank you!!

labelConfig(label)

// Given a TypographyLabelRepresentable with a single line of text
let sut = TextStyleLabel.TypographyLabelRepresentable(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same deal here, we want to pass the config to the sut as a parameter.

}
}

extension TextStyleLabel {
Copy link
Contributor

Choose a reason for hiding this comment

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

SwiftLint is complaining about this and we don't really need the internal view to be declared inside TextStyleLable. Let's remove the extension and just have TypographyLabelRepresentable be declared at top-level.

Sources/YMatterType/Elements/TextStyleLabel.swift:53:9: warning: Nesting Violation: Types should be nested at most 1 level deep (nesting)

Copy link
Contributor

@mpospese mpospese left a comment

Choose a reason for hiding this comment

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

Looks great. Gracias, Virginia!

@mpospese mpospese merged commit 10686a7 into codeandtheory:main Feb 6, 2023
@mpospese mpospese linked an issue Feb 6, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[SwiftUI] Add TypographyText
2 participants