Skip to content
This repository was archived by the owner on Feb 2, 2023. It is now read-only.

Conversation

nguyenhuy
Copy link
Contributor

My experiment on #849.

The idea is to provide easier-to-reason alignment properties that are applied to all items in stack layout. Whether an horizontal alignment means "align items" or "justify content" depends on the direction of the stack and is resolved immediately automatically. Same thing for vertical alignment.

I don't provide per-item horizontal and vertical alignments because, depends on the stack direction, only one of them can be resolved. This is because align can be applied on a per-item basis (i.e alignSelf) while justify cannot. For example, in a horizontal stack, vertical alignment equals to alignSelf while horizontal alignment is meaningless.

Another issue is that changing a stack direction after setting horizontal and/or vertical alignments will result in unexpected behaviours. Few ideas on how to handle this:

  • Store alignments and resolve them right before measurements.
  • Automatically swap alignments after a direction change.
  • Make direction read-only and can only be set via init.
  • Assert and warn user (in direction setter).
  • Make it clear in the documentation that after a direction change, horizontal and vertical alignments must be revisited.

I added some test cases to show how the new alignments can be used. Please let me know your thoughts.

EDIT: Added another (and probably the best) idea on handling direction change, hence it's listed first.

@nguyenhuy nguyenhuy force-pushed the AlignmentWrapper branch 2 times, most recently from d3052f5 to 24bcb2b Compare November 20, 2015 17:12
@rcancro
Copy link
Contributor

rcancro commented Nov 20, 2015

My hero!!!

Do you think it will be possible to create a justifySelf property on ASLayoutable?

@nguyenhuy
Copy link
Contributor Author

I think it is possible, but would require quite a large change in the way stack layout distributes violation (ASStackPositionedLayout). More importantly, what is the behaviour of justifySelf that you are having in mind?

@rcancro
Copy link
Contributor

rcancro commented Nov 20, 2015

I was mostly just thinking about being able to set horizontal/vertical alignment on a per-child basis.

@appleguy
Copy link
Contributor

@nguyenhuy I think we should prevent the stack direction from being changed. Because a layout spec locks down as immutable after it is returned anyway, there is no valid use case for changing the stack direction.

This would be a bit unusual, but let's keep the direction property as writable, but add comments that it can only be set once — and throw an assertion if it is changed after it has already been set.

Implementing justifySelf would be a huge development, as it would allow us to extend this valuable pattern to each item. I think that would be a "holy grail" of layout in ASDK, to be honest :). We should do more research on whether a) CSS / Flexbox support this, b) React or CK have evaluated supporting this (or do now?), and c) make a final decision on whether we'll aim to do this. If it looks like it will be too complicated to do, that decision itself is useful as we should look at better documenting alignSelf.

@nguyenhuy
Copy link
Contributor Author

@appleguy I agree that we should keep the direction property as writable. However, I propose a slightly different implementation approach (e61f828), because throwing an assertion if the direction is changed more than once doesn't handle all the edge cases. For example, below code would run just fine with no assertion thrown:

ASStackLayoutSpec *stack = [[ASStackLayoutSpec alloc] init];
// Horizontal and vertical alignments are determined using the default direction (horizontal)
[stack setHorizontalAlignment:ASAlignmentRight];
[stack setVerticalAlignment:ASAlignmentBottom];
// Changing the direction now doesn't throw an assertion because it is the first (external) change
// But above alignments will be invalid and need to be set again
stack.direction = ASStackLayoutDirectionVertical;

Regarding justifySelf:

  • I think CK doesn't supported it as of now, though I'm not sure if CK has evaluated supporting. Not sure about React either.
  • justifySelf is defined in CSS Box spec but not in Flexbox spec. This is probably the reason:

Flex Items:
This property does not apply to flex items, because there is more than one item in the main axis. See flex for stretching and justify-content for main-axis alignment. [CSS3-FLEXBOX]

(Source: http://www.w3.org/TR/css-align-3/#justify-self-property)

  • I think it will be cool if we decide to implement justifySelf, but we will be pretty much on our own. That's why I asked @rcancro about his idea of its behaviour, because that's the first thing we need to clearly define and I have absolutely no idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Small detail, but let's put this in the middle of the three just like Center below :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@nguyenhuy nguyenhuy force-pushed the AlignmentWrapper branch 4 times, most recently from ed7c3bc to 63e9f4e Compare November 24, 2015 18:54
@nguyenhuy
Copy link
Contributor Author

I've pushed a new diff that I think is very close to a landing state. Some new improvements compare to the old diff:

  • Comments are addressed.
  • After a direction change (including the first time), horizontal and vertical alignments are resolved again. Thus they always remain valid.
  • Assertions are added to ensure that once users opt in for alignments, they can no longer set alignItems and justifyContent directly.
  • More test cases are added to verify above behaviours.

Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor - this sentence wasn't completed. Let's patch it up in a future diff (it doesn't have to be specific to this, could even be a change to an example app). Is this format readable by AppleDoc?

@appleguy appleguy changed the title [WIP] Implement horizontal and vertical alignments [ASLayoutSpec] Implement horizontal and vertical alignments Nov 29, 2015
@appleguy
Copy link
Contributor

@nguyenhuy extremely excited about this patch - thank you!

appleguy added a commit that referenced this pull request Nov 29, 2015
[ASLayoutSpec] Implement horizontal and vertical alignments
@appleguy appleguy merged commit 3733427 into facebookarchive:master Nov 29, 2015
@appleguy appleguy added this to the 1.9.2 milestone Nov 29, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants