-
Notifications
You must be signed in to change notification settings - Fork 353
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
Added direction to allow the list to grow up #28
Conversation
Ok-- I added some comments -- will take me a while before I can actually test it in my app -- .. would be nice if we had some test stuff here :)
… On Jan 20, 2019, at 11:15 PM, Kenneth Liang ***@***.***> wrote:
@KaYBlitZ <https://github.com/KaYBlitZ> requested your review on: #28 <#28> Added direction to allow the list to grow up.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub <#28 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB8M7YP-oYfp_16DVqQPjrCPFzq9eq0wks5vFT7OgaJpZM4aKEwe>.
|
|
||
OverlayEntry _overlayEntry; | ||
|
||
bool _isOpened = false; | ||
bool widgetMounted = true; | ||
double maxHeight = defaultHeight; | ||
|
||
_SuggestionsBoxController(this.context); | ||
_SuggestionsBoxController(this.context, this.direction); | ||
|
||
open() { | ||
if (this._isOpened) return; |
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.
Also the test before (and for some reason I can't comment on that line) in close() could be better done (without the assert) with
this._overlayEntry?.remove();
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.
Which line is this?
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 fixed the assert. It should only allow down & up. No left, right, or null. As for the pubspec and the CHANGELOG files, I was looking at the previous commits and Abdul seems to modify these after merging into the develop branch. Then he brings everything back into master.
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.
Looking good -- see my comment (suggestion) about the line # I was referring too -- maybe because it wasn't changed code it wouldn't let me comment in the code .. so that's far ..
Great work @KaYBlitZ! Unfortunately I won't be able to review your work before the weekend, so if you guys are sure if the code you can go ahead and merge it :) |
I used the code from yohom and Rassell to create a new attribute direction. This allows the list to grow up or down. It is by default down.
The list will resize to be just below the notch of the device if applicable. It will auto-resize during orientation changes as well.
Let me know what you guys think and see if you can break it ;)