Skip to content

Add support for lineLimit(_ limit: Int, reservesSpace: Bool) - #161

Merged
marcprux merged 1 commit into
skiptools:mainfrom
dfabulich:text-linelimit-reservesspace
May 28, 2025
Merged

Add support for lineLimit(_ limit: Int, reservesSpace: Bool)#161
marcprux merged 1 commit into
skiptools:mainfrom
dfabulich:text-linelimit-reservesspace

Conversation

@dfabulich

@dfabulich dfabulich commented Mar 24, 2025

Copy link
Copy Markdown
Member

https://developer.apple.com/documentation/swiftui/view/linelimit(_:reservesspace:)

I've tested it with skiptools/skipapp-showcase#27 where it works on both Android and iOS.

  • REQUIRED: I have signed the Contributor Agreement
  • REQUIRED: I have tested my change locally with swift test
  • OPTIONAL: I have tested my change on an iOS simulator or device
  • OPTIONAL: I have tested my change on an Android emulator or device

@cla-bot cla-bot Bot added the cla-signed label Mar 24, 2025
@dfabulich
dfabulich force-pushed the text-linelimit-reservesspace branch 2 times, most recently from ec47d91 to 44699f6 Compare March 24, 2025 05:29
@dfabulich
dfabulich marked this pull request as ready for review March 24, 2025 05:31
@dfabulich
dfabulich force-pushed the text-linelimit-reservesspace branch from 44699f6 to 8cd4871 Compare March 24, 2025 05:40

@aabewhite aabewhite left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! Will take a look in the next couple days when I finish my current task. Off the bat, looks great, but I think I'd prefer to put lineLimit and reservesSpace together into a single Tuple (Pair in Kotlin) in the Environment. I try to group things together when it makes sense because I worry that eventually we're going to see a perf drag from so many Environment lookups

@dfabulich

Copy link
Copy Markdown
Member Author

I think a Tuple might be nicer, but I'm not sure it would make sense here, because .lineLimit is a built-in official environment value of SwiftUI, and it's an Int.

https://developer.apple.com/documentation/swiftui/environmentvalues/linelimit

The current released version of SkipUI can honor the .lineLimit environment variable today; it'll just work. If we changed the type of .lineLimit, it would break anyone who sets .environment(\.lineLimit, 2).

And if we don't change the type, then we just have to have a second environment variable. (Right?)

@aabewhite

Copy link
Copy Markdown
Contributor

Ah, I didn't notice that lineLimit is one of the exposed environment values. In that case you're absolutely right to keep them separate

@aabewhite aabewhite left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One request, otherwise looks good!

Comment thread Sources/SkipUI/SkipUI/Environment/EnvironmentValues.swift Outdated
@dfabulich
dfabulich force-pushed the text-linelimit-reservesspace branch from 8cd4871 to f63088c Compare May 28, 2025 03:45
@dfabulich

Copy link
Copy Markdown
Member Author

A couple of months later, I've resolved feedback; I think this is now ready to merge.

@marcprux
marcprux merged commit 4bafa73 into skiptools:main May 28, 2025
@marcprux

Copy link
Copy Markdown
Member

Looks great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants