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

df/RefactorFloatingPanel #74

Merged
merged 10 commits into from
Jul 18, 2022
Merged

df/RefactorFloatingPanel #74

merged 10 commits into from
Jul 18, 2022

Conversation

dfeinzimer
Copy link
Collaborator

@dfeinzimer dfeinzimer commented Jun 24, 2022

@dfeinzimer dfeinzimer added the enhancement New feature or request label Jun 24, 2022
@dfeinzimer dfeinzimer self-assigned this Jun 24, 2022
…wift

Co-authored-by: Mark Dostal <mdostal@esri.com>
@dfeinzimer dfeinzimer marked this pull request as ready for review June 27, 2022 23:09
@dfeinzimer dfeinzimer requested a review from mhdostal June 27, 2022 23:10
@dfeinzimer
Copy link
Collaborator Author

@mhdostal I've applied some changes that pull out the width param and make FP act more like how we discussed on Slack:

Simulator Screen Recording - iPad Pro (12 9-inch) (5th generation) - 2022-07-06 at 14 42 24

Simulator Screen Recording - iPhone 13 Pro Max - 2022-07-06 at 14 44 24

Achieving this is still a bit wonky though as it requires the user to conditionally apply width and padding:

Screen Shot 2022-07-06 at 14 51 15

@mhdostal
Copy link
Member

mhdostal commented Jul 7, 2022

@dfeinzimer Can we move the .padding logic into the floating panel? That way the client won't need to deal with it but they can add extra padding if they like.

As for the width, can the client simply set the .frame(maxWidth:) and let the floating panel deal with the rest? Setting just the maxWidth, would still allow the FP to shrink for compact width and limit the max width for regular width size classes.

@dfeinzimer
Copy link
Collaborator Author

Moving padding internal is a good call. I also changed width as suggested but am using

.frame(maxWidth: horizontalSizeClass == .regular ? 360 : .infinity)

because the provided value (e.g. 360 here) may not be wide enough to fill the width even in compact environments.

Copy link
Member

@mhdostal mhdostal left a comment

Choose a reason for hiding this comment

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

Just the one thing.

@dfeinzimer dfeinzimer requested a review from mhdostal July 11, 2022 21:33
mhdostal
mhdostal previously approved these changes Jul 12, 2022
@mhdostal
Copy link
Member

@dfeinzimer Nice work! These are useful additions to the FP!

@dfeinzimer dfeinzimer requested a review from philium July 12, 2022 21:09
Copy link
Contributor

@philium philium left a comment

Choose a reason for hiding this comment

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

This is what I see when I first open the example on an iPhone in portrait orientation:

image

Is that right? It seems odd that the floating panel would be taking up the whole screen. If I rotate to landscape and then back to portrait, then the floating panel takes up only half of the screen:

image

I'm guessing that's the intended behavior.

@dfeinzimer
Copy link
Collaborator Author

I'm guessing that's the intended behavior.

At the moment, yes, that's at least the expected behavior. This is actually pre-existing if you try it on v.next (minus the rotation piece).
Simulator Screen Shot - iPhone 13 Pro Max - 2022-07-15 at 12 38 05

I intend to look at this height behavior in a subsequent PR to add support for use cases where programatic control of height is needed, similar to how the active detents of a sheet can be changed.

@dfeinzimer dfeinzimer linked an issue Jul 15, 2022 that may be closed by this pull request
@mhdostal
Copy link
Member

I intend to look at this height behavior in a subsequent PR to add support for use cases where programatic control of height is needed, similar to how the active detents of a sheet can be changed.

Yes, this would be good. The original design for the Floating Panel in the Data Collection open-source app did not allow arbitrary heights, but predefined "modes" (aka, detents):

- the height of the panel would have three "modes"
    * "Summary" - the panel height would be constrained to only a few lines of text
    * "Half-height" - the panel is roughly 30-40% of the height of the available display (height is customizable)
    * "Full-height" - the panel is the full height of the display

@dfeinzimer dfeinzimer merged commit 8bd112a into v.next Jul 18, 2022
@dfeinzimer dfeinzimer deleted the df/RefactorFloatingPanel branch July 18, 2022 22:08
@dfeinzimer dfeinzimer mentioned this pull request Jul 28, 2022
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Floating Panel height can be set larger than displayed height
3 participants