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

Aligned Additions #42

Merged
merged 1 commit into from
Dec 17, 2019
Merged

Aligned Additions #42

merged 1 commit into from
Dec 17, 2019

Conversation

kyleve
Copy link
Collaborator

@kyleve kyleve commented Dec 16, 2019

  • Add 'fill' options to the 'Aligned' element, to support content taking up the full width or height in each dimension.
  • Add the UI tests to the 'SampleApp' workspace to allow running the tests.

@kyleve kyleve requested review from watt and timdonnelly December 16, 2019 18:48
@kyleve
Copy link
Collaborator Author

kyleve commented Dec 16, 2019

Hmm, Travis is stuck, looking into it:

Installing RubyGems 3.1.1
  Successfully built RubyGem
  Name: bundler
  Version: 2.1.0
  File: bundler-2.1.0.gem
bundler's executable "bundle" conflicts with /Users/travis/.rvm/rubies/ruby-2.6.4/bin/bundle
Overwrite the executable? [yN]  

@@ -15,8 +15,6 @@ matrix:
xcode_destination: platform=iOS Simulator,OS=9.3,name=iPhone 4s
before_install:
- cd SampleApp
- gem update --system
- gem install bundler
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These lines aren't needed – they're already on Travis.

@@ -14,6 +14,9 @@ public struct Aligned: Element {
case center
/// Aligns the content to the bottom edge of the containing element.
case bottom

/// The content fills the full vertical height of the containing element.
case fill
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should update the docs on this type if we add this, to something like:

For alignments other than fill, the size of the content element is determined by calling measure(in:) on the content element – even if that size is larger than the wrapping element.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

…g up the full width or height in each dimension.
@kyleve kyleve force-pushed the kve/add-fill-to-aligned branch from a41ac1d to d037231 Compare December 17, 2019 20:47
/// When using alignment mode `.fill`, the content is scaled to the width or height of the `Aligned` element.
///
/// For other modes, the size of the content element is determined by calling `measure(in:)`
/// on the content element – even if that size is larger than the wrapping element.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@watt not going to change it here, but this bit "even if that size is larger than the wrapping element." seems a bit weird to me – is there a use case for this, or should we change the implementation to cap the size?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I copied this from Centered. I think it's reasonable behavior, as it limits the responsibility of this element and favors composition (also a reason that I have mixed feelings about adding fill here instead of introducing a Fill element). If you want to cap the size of an element, composition with ConstrainedSize seems like a more scalable solution.

@kyleve kyleve merged commit dbfda59 into master Dec 17, 2019
@watt watt deleted the kve/add-fill-to-aligned branch November 24, 2021 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants