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

Unify inside & outside position alignment #617

Merged
merged 13 commits into from
Oct 10, 2023

Conversation

zsoltk
Copy link
Contributor

@zsoltk zsoltk commented Oct 8, 2023

Description

Related issue

  • Replace offset values with alignment #564 created a separate class for PositionOutside, meaning we ended up with two classes that could affect alignment: One for inside the container, one for offsetting whole screen distances for scrolling.

Problems

  • .align() can only be applied once in a Modifier change, only the first one having effect
  • Following from this, we could have either outside alignment or inside alignment, but not both
  • This blocks implementing a shared element transition from a list (outside alignment) to a detail screen (hero element changes alignment inside the screen)
  • Adding outside alignment bias into Position isn't possible since it already maxed out 4D for its AnimationVector (2 for alignment bias, 2 for relative offset coordinates), a hard limit imposed by Compose.

Approach in this PR

  • Extract out the relative offset to a standalone property: PositionOffset which only takes a DpOffset
  • This frees up 2 dimensions in the AnimationVector that we can use for outside alignment instead
  • Merge PositionInside and PositionOutside 2+2 alignment bias values into a single property PositionAlignment
  • The new property can account for both inside and outside alignment in a single modifier correctly

Migration steps for client code

In any client code TargetUiState instances:

  • Replace usages of PositionInside and PositionOutside with PositionAlignment
  • Wherever an extra offset was also used next to alignments, also introduce a new field for PositionOffset and move the value there

Check list

  • I have updated CHANGELOG.md if required.
  • I have updated documentation if required.

@zsoltk zsoltk added enhancement New feature or request appyx-interactions labels Oct 8, 2023
@zsoltk zsoltk added this to the 2.0 milestone Oct 8, 2023
Comment on lines +141 to +145
val value = renderValueFlow.collectAsState()
val boxScope = requireNotNull(LocalBoxScope.current)
with(boxScope) {
this@composed
.align(value.value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
val value = renderValueFlow.collectAsState()
val boxScope = requireNotNull(LocalBoxScope.current)
with(boxScope) {
this@composed
.align(value.value)
val value = renderValueFlow.collectAsState()
val boxScope by requireNotNull(LocalBoxScope.current)
with(boxScope) {
this@composed
.align(value)

@zsoltk zsoltk merged commit df20f1a into bumble-tech:2.x Oct 10, 2023
7 checks passed
@zsoltk zsoltk deleted the unify-position-alignment branch October 10, 2023 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
appyx-interactions enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants