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

feat: variable front page height #73

Merged
merged 16 commits into from Feb 22, 2021

Conversation

mockturtl
Copy link
Contributor

@mockturtl mockturtl commented Jan 29, 2021

Adds double frontLayerActiveFactor to BackdropScaffold. See #70.

Optional: use drawerScrimColor to disable the back layer when it is partially concealed.

Default values leave existing behavior unchanged

Screenshot from 2021-01-28 20-51-43

Screenshot from 2021-01-28 20-54-59

New behavior

  • Using drawerScrimColor: Colors.black54, frontLayerActiveFactor: 0.2 (20% height)

Screenshot from 2021-01-28 20-54-51

  • Using drawerScrimColor: Colors.black54, frontLayerActiveFactor: 0.5 (50% height)

Screenshot from 2021-01-28 20-51-59

@mockturtl
Copy link
Contributor Author

@daadu Sorry for the delay! Let me know what you think of these changes and I can add the documentation changes discussed in the linked issue.

lib/scaffold.dart Outdated Show resolved Hide resolved
@daadu daadu requested a review from WieFel January 29, 2021 05:09
@WieFel
Copy link
Collaborator

WieFel commented Jan 29, 2021

@mockturtl what's the purpose of setting frontLayerActiveFactor to some percent value (like 0.5)?
I don't think that there is much necessity for setting percentual height of the front layer, at least in common use cases.

Wasn't your original issue #70 about the front layer that should fill exactly the height of its content?
In that case, a flag (something like adaptiveFrontLayerHeight: [ true | false ]) would suffice and could be internally used to adapt the front layer's height depending on its content, just as it is already done with the back layer and stickyFrontLayer property.

@mockturtl
Copy link
Contributor Author

I don't think that there is much necessity for setting percentual height of the front layer, at least in common use cases.

I agree it would be nice if Material provided more guidance here. I can't cite any use cases of Backdrop outside the "Material studies" apps. (Mine is functionally equivalent to a drawer, as in Shrine.)

Side note, I do think a "fractional" parameter is more user-friendly than a pixel value.

a flag (something like adaptiveFrontLayerHeight: [ true | false ]) would suffice and could be internally used to adapt the front layer's height depending on its content, just as it is already done with the back layer and stickyFrontLayer property.

That's arguably a nicer API. Can we make it work? I get layout flicker when I naively wrap the front layer with _MeasureSize, and it never settles.

I would guess Column is the most typical front layer. There could be a solution where we insist front layer widgets have constraints, but it seems less intuitive.

@daadu
Copy link
Member

daadu commented Feb 2, 2021

@WieFel #70 as I see, is about having "extra" drop by-default if a user wants, instead of sticking to appbar by default.

I think frontLayerActiveFactor with percent value is the correct solution for it.

What you are suggesting adaptiveFrontLayerHeight can be added independently - the behaviour would be the same as, headerHeight and stickyFrontLayer that we have now for backLayerRevealed state but frontLayerActiveFactor and adaptiveFrontLayerHeight could be for backLayerConcealed state (only difference is headerHeight is in pixcel while frontLayerActiveFactor is in %).

@WieFel
Copy link
Collaborator

WieFel commented Feb 3, 2021

I think frontLayerActiveFactor with percent value is the correct solution for it.

Ok, so please go ahead with it @mockturtl @daadu

@mockturtl mockturtl requested a review from daadu February 5, 2021 22:46
@mockturtl mockturtl marked this pull request as ready for review February 8, 2021 02:07
Copy link
Member

@daadu daadu left a comment

Choose a reason for hiding this comment

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

Some minor documentation changes and a minor refactor requested.

lib/scaffold.dart Outdated Show resolved Hide resolved
lib/scaffold.dart Outdated Show resolved Hide resolved
lib/scaffold.dart Outdated Show resolved Hide resolved
lib/scaffold.dart Outdated Show resolved Hide resolved
lib/scaffold.dart Outdated Show resolved Hide resolved
lib/scaffold.dart Outdated Show resolved Hide resolved
lib/scaffold.dart Show resolved Hide resolved
lib/scaffold.dart Outdated Show resolved Hide resolved
@daadu
Copy link
Member

daadu commented Feb 14, 2021

@WieFel Can you review it too?

mockturtl and others added 2 commits February 14, 2021 17:22
Co-authored-by: Harsh Bhikadia <harsh.bhikadiya@gmail.com>
Copy link
Collaborator

@WieFel WieFel left a comment

Choose a reason for hiding this comment

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

@mockturtl has the implementation been tested against common use-cases? It should not break any previously working functionality...

lib/scaffold.dart Outdated Show resolved Hide resolved
@mockturtl mockturtl requested a review from daadu February 21, 2021 01:08
mockturtl added a commit to mockturtl/backdrop that referenced this pull request Feb 21, 2021
Copy link
Member

@daadu daadu left a comment

Choose a reason for hiding this comment

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

@mockturtl We have a "demo" app that show cases various use-cases of backdrop implemented with the package check live demo. Feel free to add a use-case using this feature if you want in future.

Apart from that this LGTM.

@daadu daadu requested a review from WieFel February 21, 2021 10:10
@daadu daadu merged commit e48c83b into fluttercommunity:master Feb 22, 2021
@daadu
Copy link
Member

daadu commented Feb 23, 2021

This has landed with v0.5.0 on pub.dev. @mockturtl Thanks for your contribution. Please check yourself in README!!

@daadu daadu linked an issue Feb 27, 2021 that may be closed by this pull request
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.

allow variable height front pages
3 participants