-
Notifications
You must be signed in to change notification settings - Fork 330
Bug 1854131 - Three-dot main menu falls off screen, disappears in landscape mode #5663
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.
Great job @AdrianaMaries, this fix looks good to me. Can we add a unit test also please to assert this new condition? 🙏
62eaa89
to
20b0c79
Compare
@t-p-white Added a unit test to assert the new condition. |
Thanks @AdrianaMaries! Sorry for the delay, should we also have a test for availableHeightToBottom is less than availableHeightToTop? |
aad161f
to
08d1955
Compare
@t-p-white Please see my changes. Thanks. |
} | ||
|
||
@Test | ||
fun `GIVEN inferMenuPositioningData WHEN availableHeightToTop is bigger than availableHeightToBottom THEN it returns a new MenuPositioningData populated with all data needed to show a PopupWindow that fits down`() { |
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.
should this be:
fun `GIVEN inferMenuPositioningData WHEN availableHeightToTop is bigger than availableHeightToBottom THEN it returns a new MenuPositioningData populated with all data needed to show a PopupWindow that fits down`() { | |
fun `GIVEN inferMenuPositioningData WHEN availableHeightToTop is bigger than availableHeightToBottom THEN it returns a new MenuPositioningData populated with all data needed to show a PopupWindow that fits up`() { |
?
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.
@t-p-white Yes. Updated.
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 @AdrianaMaries! Just a minor comment from me
…dscape mode The popup doesn't fits up or fits down, because both availableHeightToTop and availableHeightToBottom are smaller than containerHeight. Because of this the popup is not displayed. As the popup is scrollable we should display it where the height is bigger (top or bottom).
@t-p-white Please see my changes. |
Thanks @AdrianaMaries! lgtm 😎 With the move to the monorepo this PR will be closed and your changes will be included in https://phabricator.services.mozilla.com/D205207 |
Bug 1854131 - Three-dot main menu falls off screen, disappears in landscape mode
The popup doesn't fits up or fits down, because both availableHeightToTop and availableHeightToBottom are smaller than containerHeight. Because of this the popup is not displayed. As the popup is scrollable we should display it where the height is bigger (top or bottom).
https://bugzilla.mozilla.org/show_bug.cgi?id=1854131
Pull Request checklist
After merge
To download an APK when reviewing a PR (after all CI tasks finished running):
Checks
at the top of the PR page.firefoxci-taskcluster
group on the left to expand all tasks.build-apk-{fenix,focus,klar}-debug
task you're interested in.View task in Taskcluster
in the newDETAILS
section.GitHub Automation
https://bugzilla.mozilla.org/show_bug.cgi?id=1854131