-
Notifications
You must be signed in to change notification settings - Fork 1.3k
For #12287: Add Synced Tabs to Tabs Tray #13893
Conversation
The parts I'm most unsure of are the navigation graph additions. |
Codecov Report
@@ Coverage Diff @@
## master #13893 +/- ##
============================================
+ Coverage 29.76% 29.90% +0.13%
- Complexity 1117 1125 +8
============================================
Files 422 425 +3
Lines 17204 17293 +89
Branches 2232 2241 +9
============================================
+ Hits 5121 5171 +50
- Misses 11709 11747 +38
- Partials 374 375 +1
Continue to review full report at Codecov.
|
89c2ea6
to
fa950dc
Compare
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.
Pulled this down and looks good overall, but I noticed a few things:
- Do we want this to show in the private tab in the tabs tray?
- We probably want to hide this in multiselect mode like we do for the save to collection button because otherwise it looks a bit strange?
- When opening the tabs tray with synced tabs enabled and 1 open tab, it seems to jump to the synced tabs section and opens at an awkward point in the list.
- Sometimes (can't repro 100%) it even opens further down on the synced tabs list and I would have to scroll way up to see open tabs. When I swipe up to try to scroll, it closes. Got a video from after it opened.
Moving this into it's own Tabs Tray Tab (phew mouthful) would likely solve all these problems, so if we decide to move that way (via this comment #12287 (comment) ) we can probably disregard these issues.
cc @topotropic
Thanks for the review; super helpful!
We currently don't have it in private tabs similar to the 'Add to collections' button, although I'll leave it up to @topotropic to comment (and to also consider the separate tab idea that Sorin mentioned).
Fixed! Check out the last commit that addresses the review comments.
These two are related and are a side-effect to how the tabs tray scrolls to the selected tab. The hacky approach that we currently have (calculate the index based on the adapter size), does not consider that the adapter is loaded async and we need to scroll when all the adapters are complete. I've filed #13905 to address this and added it as a blocker for removing the feature flag.
Yes! I'd really like us to look into this but I don't think it'll block us from landing this change. |
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.
Sounds good, I don't think the UX questions/bugs should block the flagged feature!
The feature is put behind a feature flag from the Secret Settings. Might be easier to review per commit.
Abstract design link to verify against.
Pull Request checklist
To download an APK when reviewing a PR: