Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

For #15543 - Expand / collapse tabs tray depending on all tabs #15749

Merged
merged 1 commit into from Oct 21, 2020
Merged

For #15543 - Expand / collapse tabs tray depending on all tabs #15749

merged 1 commit into from Oct 21, 2020

Conversation

Mugurell
Copy link
Contributor

@Mugurell Mugurell commented Oct 7, 2020

Now check all tabs - private or normal and decide whether to show the tabs tray
as expanded or collapsed depending on the highest number of tabs in any
category.
Also added support for the grid based tabs tray - will show tabs tray as
expanded if there are more than 3 or more tabs (normal or private) open
(this meaning two rows in the grid layout).

Pull Request checklist

  • Tests: No tests. Small changes in a not yet tested component.
  • Screenshots: Tried to record screen but with this involving private mode much of the video is blaked out.
  • Accessibility: The code in this PR or does not include any user facing features
List based layout Grid based layout
video video

To download an APK when reviewing a PR:

  1. click on Show All Checks,
  2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
  3. click on the "Fenix - assemble" task, then click "Run Artifacts".
  4. the APK links should be on the left side of the screen, named for each CPU architecture

@Mugurell Mugurell requested review from a team as code owners October 7, 2020 13:20
@Mugurell Mugurell changed the title For #14974 - Collapse or expand tab tray when orientation / tabs change For #15543 - Collapse or expand tab tray when orientation / tabs change Oct 8, 2020
@codecov-io
Copy link

codecov-io commented Oct 8, 2020

Codecov Report

Merging #15749 into master will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             master   #15749    +/-   ##
==========================================
  Coverage     29.76%   29.76%            
+ Complexity     1191     1185     -6     
==========================================
  Files           453      453            
  Lines         18476    18496    +20     
  Branches       2539     2383   -156     
==========================================
+ Hits           5500     5506     +6     
- Misses        12557    12595    +38     
+ Partials        419      395    -24     
Impacted Files Coverage Δ Complexity Δ
...org/mozilla/fenix/tabtray/TabTrayDialogFragment.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...main/java/org/mozilla/fenix/tabtray/TabTrayView.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...in/java/org/mozilla/fenix/settings/SupportUtils.kt 70.58% <0.00%> (-6.50%) 11.00% <0.00%> (-2.00%)
...zilla/fenix/session/VisibilityLifecycleCallback.kt 0.00% <0.00%> (-6.25%) 0.00% <0.00%> (ø%)
app/src/main/java/org/mozilla/fenix/Config.kt 50.00% <0.00%> (-4.55%) 2.00% <0.00%> (ø%)
...ry/bookmarks/BookmarkDeselectNavigationListener.kt 83.33% <0.00%> (-2.39%) 9.00% <0.00%> (ø%)
...g/mozilla/fenix/components/metrics/MetricsUtils.kt 68.75% <0.00%> (-1.57%) 8.00% <0.00%> (ø%)
...pp/src/main/java/org/mozilla/fenix/HomeActivity.kt 5.72% <0.00%> (-1.06%) 11.00% <0.00%> (-3.00%)
...la/fenix/components/metrics/AppStartupTelemetry.kt 97.05% <0.00%> (-0.67%) 22.00% <0.00%> (-1.00%)
...in/java/org/mozilla/fenix/components/Components.kt 28.94% <0.00%> (-0.64%) 1.00% <0.00%> (ø%)
... and 90 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 b1bc024...ba0be92. Read the comment docs.

@Mugurell
Copy link
Contributor Author

Mugurell commented Oct 9, 2020

Rebased after the grid layout changes for tabs tray.
Seems like the layout is still WIP so I didn't know if I should do anything to support it atm but tagging @gabrielluong for visibility of the changes that will need tp be supported also by the new layout.

@gabrielluong
Copy link
Member

Rebased after the grid layout changes for tabs tray.
Seems like the layout is still WIP so I didn't know if I should do anything to support it atm but tagging @gabrielluong for visibility of the changes that will need tp be supported also by the new layout.

Thanks for the ping! I will test this tomorrow morning and see how it interacts. I didn't see any UX feedback from @topotropic in the bugs mentioned, and an alarm kinda goes off internally for me when I see these kind of scenarios where we end up working on something prior to any UX investigation, which I always think becomes an awkward scenario for all of us. Would you be able to ping her to get some feedback? I could also be mistaken and that these conversations already took place, and if that is the case, let me apologize in advance.

@Mugurell
Copy link
Contributor Author

Thanks for the ping! I will test this tomorrow morning and see how it interacts. I didn't see any UX feedback from @topotropic in the bugs mentioned, and an alarm kinda goes off internally for me when I see these kind of scenarios where we end up working on something prior to any UX investigation, which I always think becomes an awkward scenario for all of us. Would you be able to ping her to get some feedback? I could also be mistaken and that these conversations already took place, and if that is the case, let me apologize in advance.

This patch just comes to complement what we're already doing when the tab tray first shows up.
It can take the entire screen or just half depending on how many tabs there are to show.
But we didn't account for different tab tray sizes, expanded tabs tray needed for normal tabs and collapsed tray needed for private tabs for example.
Will ask for UX feedback in the ticket.

@gabrielluong gabrielluong added the needs:UX-feedback Needs UX Feedback label Oct 13, 2020
@gabrielluong
Copy link
Member

gabrielluong commented Oct 13, 2020

@topotropic Would you be able to give this a try and provide any feedback? I personally think it looks good, but I noticed that we don't scroll to the selected tab when we switch between normal and private tabs.

@gabrielluong
Copy link
Member

I think it's also somewhat safe to say the grid layout is in a somewhat stable state. We are only working to align it more with the designs (something that I am not really good at).

@Mugurell
Copy link
Contributor Author

I think it's also somewhat safe to say the grid layout is in a somewhat stable state. We are only working to align it more with the designs (something that I am not really good at).

For showing the tab tray as collapsed / expanded we just counted the number of tabs to be displayed.
In the list based layout the minimum number of tabs for which we'll show the tab tray is 4.
Maybe for the grid based layout we should show tabs tray as expanded if there are 3 or more tabs to be shown?
Guess these are questions for @topotropic also.
With >=2 this is how it'd look.

I think we could also look into using another more suited height for the grid based collapsed tabs tray but if the changes are bigger I'd say it would be best to have a new ticket specifically for this.

@gabrielluong gabrielluong self-requested a review October 14, 2020 17:38
@gabrielluong gabrielluong self-assigned this Oct 14, 2020
@gabrielluong gabrielluong added the needs:review PRs that need to be reviewed label Oct 14, 2020
@gabrielluong
Copy link
Member

@Mugurell let's add a gif for @topotropic so she can review it quicker although my experience with trying out this PR is that it's best to try it with an apk.

@Mugurell Mugurell changed the title For #15543 - Collapse or expand tab tray when orientation / tabs change For #15543 - Expand / collapse tabs tray depending on all tabs Oct 20, 2020
@Mugurell
Copy link
Contributor Author

@Mugurell let's add a gif for @topotropic so she can review it quicker although my experience with trying out this PR is that it's best to try it with an apk.

Updated the patch to set tabs tray as expanded / collapsed only when first showing on the screen, not also when changing what tabs to show.
Added gif / video for the behaviors of both the list based and grid based tabs trays.
An apk on which these changes can be tested is available from the CI tasks - https://firefox-ci-tc.services.mozilla.com/tasks/ekFcvlBQSU-FSGPPXlbTQw#artifacts

Copy link

@topotropic topotropic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@gabrielluong gabrielluong added ux:approved and removed needs:UX-feedback Needs UX Feedback labels Oct 21, 2020
@Mugurell
Copy link
Contributor Author

I see test-debug failing because of [taskcluster:error] Task timeout after 7200 seconds. Force killing container.
Restarting CI tasks.

@Mugurell Mugurell closed this Oct 21, 2020
@Mugurell Mugurell reopened this Oct 21, 2020
Copy link
Member

@gabrielluong gabrielluong left a comment

Choose a reason for hiding this comment

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

r+ assuming we make the changes.

@gabrielluong gabrielluong added pr:approved PR that has been approved pr:waiting-for-authors PR that has been approved and awaiting any changes before they can land and removed needs:review PRs that need to be reviewed labels Oct 21, 2020
Now check all tabs - private or normal and decide whether to show the tabs tray
as expanded or collapsed depending on the highest number of tabs in any
category.
Also added support for the grid based tabs tray - will show tabs tray as
expanded if there are more than 3 or more tabs (normal or private) open
(this meaning two rows in the grid layout).
@gabrielluong gabrielluong removed the pr:waiting-for-authors PR that has been approved and awaiting any changes before they can land label Oct 21, 2020
@gabrielluong gabrielluong added the pr:needs-landing PRs that are ready to land [Will be merged by Mergify] label Oct 21, 2020
@gabrielluong gabrielluong reopened this Oct 21, 2020
@gabrielluong gabrielluong merged commit 358ca2c into mozilla-mobile:master Oct 21, 2020
@Mugurell Mugurell deleted the 15543TabTray branch October 30, 2020 16:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:approved PR that has been approved pr:needs-landing PRs that are ready to land [Will be merged by Mergify]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants