Repository navigation
Conversation
Refs #2218, reported by @bartmuskala, who picked this out of a screenshot pair. They raised the same thing on #1068 and got no reply before it was wrapped up. iOS has five tabs and Android had four: iOS Today · Trends · Sleep · Coach · More Android Today · Trends · Sleep · More Coach was promoted to a top-level tab on iOS and this side did not follow, so it sat inside More while the Android shell's own comment claimed the two bars matched. Both of those comments were stale and now say what is true. Four parts, because the tab slot alone would have been wrong in three ways. The slot itself, in iOS order, using AutoAwesome: the sparkles glyph iOS uses and the one the More row already showed, so an entry a wearer has learned keeps its face when it moves. The More slot's selected state. It spelled its siblings out by name, so Coach would have lit Coach AND More together. It now derives from the bar's own lists, which is why spelling them out was what made this a two-part change in the first place. Coach removed from the More sheet. iOS says it plainly at the promotion: "K3: Coach promoted to a top-level tab, no longer listed under More." Android's drawer comment says bar tabs are not listed "exactly as on iOS", so leaving it would have put one destination in two places and broken the invariant the list documents about itself. The label bounded to one line. A fifth slot takes about a fifth off every label's width and the bar scale goes to 2x, so the longest of them can no longer be assumed to fit on a narrow phone. Wrapping breaks nothing, since barHeight is measured afterwards, but a two-line nav bar at one size and a one-line bar at the next is the kind of thing nobody reports and everybody notices. No new strings: nav_coach already exists with every locale it needs. com.noop.ui 1054 tests 0 failures, compileFullDebugKotlin clean, i18n audit clean, doc_comment_lint clean. Android only. This closes a gap where iOS moved and Android did not; it is not an argument that the two must match everywhere, and the issue says so.
Third re-review pass on #2222. The PR said the four parts were checked by reading and by the compiler, with nothing in the suite pinning them. That was honest and it was also the wrong place to stop, because the part that nearly shipped wrong is the one a reader cannot see. Leaving Coach in the More sheet after promoting it to a tab would have put one destination in two places. The two lists sit four hundred lines apart and neither mentions the other, so nothing about reading either one suggests checking the other. Only the first re-review caught it, and only because I went looking. barLeadingTabs, barTrailingTabs, drawerGroups and the two data classes are `internal` now rather than `private`, which is the whole cost of testing them: they are plain data, so the assertions run in the ordinary JVM suite with no Compose harness, which this repo does not have. Four cases. The bar and the More sheet are disjoint. The bar carries exactly Today, Trends, Sleep, Coach, in iOS order, with More appended by the bar itself. No destination occupies two slots, which the More slot's selected state now depends on. Every tab has a label resource, so no slot can render an empty word or an empty accessibility description. Control: putting Coach back into the Insights group fails noBarTabIsAlsoListedInTheMoreSheet with "found [Coach]". AppRoot.kt was restored byte-for-byte afterwards. Also checked this pass and left alone: BarSlot sets its accessibility description from the full label rather than from the rendered Text, so the one-line ellipsis added earlier truncates what is drawn and not what TalkBack reads. That is the right arrangement and it was already there. com.noop.ui 1058 tests 0 failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #2218, reported by @bartmuskala, who picked this out of a screenshot pair. They raised the same thing on #1068 and got no reply before it was wrapped up.
iOS has five tabs and Android had four:
Coach was promoted to a top-level tab on iOS and this side did not follow, so it sat inside More while the Android shell's own comment claimed the two bars matched:
Four parts, because the slot alone would have been wrong three ways
The slot, in iOS order, using
AutoAwesome: the sparkles glyph iOS uses and the one the More row already showed, so an entry a wearer has learned keeps its face when it moves.The More slot's selected state. It spelled its siblings out by name, so Coach would have lit Coach and More together. It now derives from the bar's own lists, which is exactly why spelling them out made this a two-part change.
Coach out of the More sheet. iOS says it plainly at the promotion: "K3: Coach promoted to a top-level tab, no longer listed under More." Android's drawer comment says bar tabs are not listed "exactly as on iOS", so leaving it would have put one destination in two places and broken the invariant that list documents about itself.
The label bounded to one line. A fifth slot takes about a fifth off every label's width, and the bar scale goes to 2x, so the longest of them can no longer be assumed to fit on a narrow phone. Wrapping breaks nothing, since
barHeightis measured afterwards and screens clear whatever it comes to, but a two-line nav bar at one size and a one-line bar at the next is the kind of thing nobody reports and everybody notices.Verification
com.noop.ui: 1054 tests, 0 failures.compileFullDebugKotlin,i18n_audit --cianddoc_comment_lintall clean. No new strings:nav_coachalready exists with every locale it needs.Not covered by a test, and worth saying so rather than implying otherwise: there is nothing in the suite that pins the bar's tab set, so the four parts above are checked by reading and by the compiler, not by a regression test. A test asserting the bar and the More list are disjoint would have caught the duplication I nearly shipped, and is worth adding, but it wants a Compose-capable harness this repo does not have.
Scope
Android only, and deliberately narrow. This closes a gap where iOS moved and Android did not. It is not an argument that the two must match everywhere, and the issue says so: the Android sleep hero's "in bed" window differs from iOS on purpose (#160, #395) and aligning it would be a regression dressed as consistency.