Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

For #10005 - Replace layout manager to effectively change stackFromEnd #10006

Merged
merged 2 commits into from
Apr 6, 2021

Conversation

Mugurell
Copy link
Contributor

@Mugurell Mugurell commented Apr 1, 2021

For #10005

The stackFromEnd property is not initially set even though
endOfMenuAlwaysVisible might be true since when having a bottom expandable
menu, that should always be scrolled to the top.
And we know if we're in that situation only later, after menu is configured and
based on it it can be inferred if it should be collapsed or not.

Setting stackFromEnd only after this steps, even if the menu is not yet on
the screen seems to have no effect.

The only solution is to replace menu's layout manager with one that has this
stackFromEnd from the beginning.

full height menu recording collapsible menu recording
FullHeightMenu.mp4
CollpasedMenu.mp4

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@Mugurell Mugurell requested a review from a team as a code owner April 1, 2021 11:21
@codecov
Copy link

codecov bot commented Apr 1, 2021

Codecov Report

Merging #10006 (e5ca5a4) into master (f39d1f4) will increase coverage by 1.30%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #10006      +/-   ##
============================================
+ Coverage     74.12%   75.42%   +1.30%     
+ Complexity     5833     1375    -4458     
============================================
  Files           786      225     -561     
  Lines         29199     6255   -22944     
  Branches       4799      936    -3863     
============================================
- Hits          21644     4718   -16926     
+ Misses         5046     1136    -3910     
+ Partials       2509      401    -2108     
Impacted Files Coverage Δ Complexity Δ
...ava/mozilla/components/browser/menu/BrowserMenu.kt 71.02% <100.00%> (+0.83%) 28.00 <2.00> (+2.00)
...a/components/service/fxa/FxaDeviceConstellation.kt
...e/digitalassetlinks/api/CheckAssetLinksResponse.kt
...ain/java/mozilla/components/service/glean/Glean.kt
.../feature/downloads/SimpleDownloadDialogFragment.kt
...lla/components/feature/top/sites/TopSitesConfig.kt
...n/java/mozilla/components/service/pocket/Logger.kt
...ava/mozilla/components/lib/dataprotect/KeyUtils.kt
...mponents/lib/crash/prompt/CrashReporterActivity.kt
... and 553 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f39d1f4...e5ca5a4. Read the comment docs.

@Mugurell Mugurell added the 🕵️‍♀️ needs review PRs that need to be reviewed label Apr 1, 2021
@Amejia481 Amejia481 self-requested a review April 5, 2021 14:59
@eliserichards eliserichards linked an issue Apr 5, 2021 that may be closed by this pull request
Copy link
Contributor

@Amejia481 Amejia481 left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽
Just one nit, let add an entry to the changelog file.

…ge stackFromEnd

The `stackFromEnd` property is not initially set even though
`endOfMenuAlwaysVisible` might be true since when having a bottom expandable
menu, that should always be scrolled to the top.
And we know if we're in that situation only later, after menu is configured and
based on it it can be inferred if it should be collapsed or not.

Setting `stackFromEnd` only after this steps, even if the menu is not yet on
the screen seems to have no effect.

The only solution is to replace menu's layout manager with one that has this
`stackFromEnd` from the beginning.
@Mugurell Mugurell force-pushed the endOfMenuAlwaysVisible branch from c61a60f to e05792a Compare April 6, 2021 13:17
@Mugurell Mugurell added 🛬 needs landing PRs that are ready to land and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Apr 6, 2021
@mergify mergify bot merged commit 34f557d into mozilla-mobile:master Apr 6, 2021
@Mugurell Mugurell deleted the endOfMenuAlwaysVisible branch April 6, 2021 16:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🛬 needs landing PRs that are ready to land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BrowserMenu - endOfMenuAlwaysVisible = true has no effect
2 participants