-
Notifications
You must be signed in to change notification settings - Fork 593
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
[Adaptive] Add accompanist/adaptive library with TwoPane implementation. #1281
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Alex. Did a high level API pass, left a few comment.
Great work!
fallbackStrategy: TwoPaneStrategy, | ||
windowGeometry: WindowGeometry, | ||
): TwoPaneStrategy = TwoPaneStrategy { density, layoutDirection, layoutCoordinates -> | ||
val verticalFold = windowGeometry.displayFeatures.find { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and everywhere else in the TwoPane we only use DisplayFeatures, and window size class from window geometry is used for a completely separate type of decision making.
Given the above, does that make sense for display features and WSC to be combined into the same interface? Splitting them would make the contracts easier to track in all places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that's true and makes sense, I was trying to keep them together as similarly-scoped pieces of information. I think this relates to a larger CompositionLocal
question:
If we keep them separate, then that's asking for more window-layout information to be passed around everywhere in the app through different layers. But we keep explicitness, and it's easy to replace in previews and tests.
If we instead provide them in a CompositionLocal
, then we lose the explicitness,and previews and testing is more difficult (needing to override a CompositionLocal
instead of passing it manually). But, we avoid having to pass them through everywhere, and we can also make it easier to have the strategies be fold-aware by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the split, I think splitting them we don't necessarily ask for more movements, but it might feel like because we operate with two entities instead of one. With that in mind, we might notice that we don't propagate WSC where it is not needed, so in that matter we actually have better-SRP-ed API that asks only for what it needs.
CompositionLocal, as you mentioned, adds some implicit-y and we can always fall back to this option if we want to. Do you think it would make sense to try without it? If developers want, they can do it themselves easy-peasy. Also if you want to read this composition local, you will have to deal with the @composables everywhere here in the API, which I'm not sure is a good idea for such a flexible API surface as we have right now.
Could you try to split them and we can see how it goes? Maybe then it would warrant for a composition local for the display features or the object that contains them? And we can keep it as a parameter on the two pane so that we can default to the composition local, and allow to override for previews and tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to split up WindowGeometry
, and depend just on the list of DisplayFeature
s!
adaptive/src/main/java/com/google/accompanist/adaptive/TwoPane.kt
Outdated
Show resolved
Hide resolved
adaptive/src/main/java/com/google/accompanist/adaptive/TwoPane.kt
Outdated
Show resolved
Hide resolved
adaptive/src/main/java/com/google/accompanist/adaptive/TwoPane.kt
Outdated
Show resolved
Hide resolved
eda9b9c
to
26d1a55
Compare
dfd4bc3
to
98139d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, we are getting close! A few more commentarinos
adaptive/src/main/java/com/google/accompanist/adaptive/TwoPane.kt
Outdated
Show resolved
Hide resolved
*/ | ||
public fun TwoPaneStrategy( | ||
windowGeometry: WindowGeometry, | ||
defaultStrategy: TwoPaneStrategy, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the name default
more. What I was wondering if we need this wrapping at all? Having to create a strategy and provide a default one for the non-fold seems like an unnecessary step for developers. They might (should) use the other ones, but this function take the best name in the space.
What do you think if we have just remove this from public api?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we remove this one from the public API, then I think we would need to expose how to create the fold strategies to allow for the same behavior, which would also mean exposing the idea of a strategy that may not apply. Not sure if we'll be able to completely avoid having a default
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the latest commit, I was able to keep current functionality intact while removing the direct fallback support from the public API. Let me know what you think!
* Otherwise, the gap will be placed at the given [splitFraction] from start, with the given | ||
* [gapWidth]. | ||
*/ | ||
public fun HorizontalTwoPaneStrategy( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably half the number of APIs if you provide an orientation: Orientation parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer to keep it is as-is. Having the name split means the parameter names can be orientation specific (gapWidth
vs gapHeight
) and clearer behavior when it comes to LTR and RTL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack, let's roll with it!
public fun HorizontalTwoPaneStrategy( | ||
windowGeometry: WindowGeometry, | ||
splitOffset: Dp, | ||
offsetFromStart: Boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this might have a reasonable default, perhaps, true
?
Also, I cannot understand why, but having this boolean makes me want to split the functions into FixedStartTwoPaneStrategy(orientation:Orientation, ...)
and FixedEndTwoPaneStrategy
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to have default of true
!
fallbackStrategy: TwoPaneStrategy, | ||
windowGeometry: WindowGeometry, | ||
): TwoPaneStrategy = TwoPaneStrategy { density, layoutDirection, layoutCoordinates -> | ||
val verticalFold = windowGeometry.displayFeatures.find { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the split, I think splitting them we don't necessarily ask for more movements, but it might feel like because we operate with two entities instead of one. With that in mind, we might notice that we don't propagate WSC where it is not needed, so in that matter we actually have better-SRP-ed API that asks only for what it needs.
CompositionLocal, as you mentioned, adds some implicit-y and we can always fall back to this option if we want to. Do you think it would make sense to try without it? If developers want, they can do it themselves easy-peasy. Also if you want to read this composition local, you will have to deal with the @composables everywhere here in the API, which I'm not sure is a good idea for such a flexible API surface as we have right now.
Could you try to split them and we can see how it goes? Maybe then it would warrant for a composition local for the display features or the object that contains them? And we can keep it as a parameter on the two pane so that we can default to the composition local, and allow to override for previews and tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Alex, LGTM given the comments are addressed, I think this is a good API to try out!
I took a look at the layouting logic impl-wise, seems pretty straighforward. If you want me to take a look at some other implementation bits - let me know
* Otherwise, the gap will be placed at the given [splitFraction] from start, with the given | ||
* [gapWidth]. | ||
*/ | ||
public fun HorizontalTwoPaneStrategy( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack, let's roll with it!
strategy: TwoPaneStrategy, | ||
displayFeatures: List<DisplayFeature>, | ||
modifier: Modifier = Modifier, | ||
foldAwareConfiguration: FoldAwareConfiguration = FoldAwareConfiguration.AllFolds, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why this is not on the public TwoPaneStrategies? Do you think it would be a better API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this was on each of the TwoPaneStrategy
s, it would be a bit more awkward to define a custom TwoPaneStrategy
since it would have to be fold aware. Placing it on the single TwoPane
entry point means that at least one type of fold has to be handled, and the rest of the strategies don't need to directly care about folds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Sets up an
adaptive
library, with the intention of adding a collection of adaptive layout utilities to help optimize UI for smaller and larger screens.The first utility added is
TwoPane
, which allows placing two slots of content with their placement controlled by a specific strategy. These strategies can be composed, and the built-in entry point supports positioning the slots around folding features automatically.TwoPane
itself does not support showing one slot or the other: if that behavior is desired (because there isn't enough room to show both slots), then that decision should be made a level above theTwoPane
.Some of those more complicated layouts (list/detail, supporting panel) would then be using
TwoPane
as an implementation detail, and configuring theTwoPane
strategy and what is passed to the slots appropriately.This PR is using
a snapshotthe latest version of Compose for now to have access toPlaceable.coordinates
, since folding features are given in window coordinates, soTwoPane
needs to know it's location relative to the window in order to know where to position its slots.To provide information about the folding features, I've also set up
aa calculation for the list of display features.WindowGeometry
object which contains information about the window. Right now, this includes the list of display features, as well as the window size class. The window size class isn't used anywhere right now, but I included it to have a single object containing the "top-level" window state that can be passed around.device-2022-08-04-182836.mp4