-
Notifications
You must be signed in to change notification settings - Fork 350
home: Add icons label on main navigation bar. #1879
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
There is a lot of text to read in that PR description. Did you use ChatGPT or another LLM to write it? If so, we'd much rather see whatever prompt you used, instead of the LLM output — the LLM fundamentally doesn't add any information that wasn't there already, and the prompt should be a lot shorter and so less work to read. There's also no need to repeat information that's there in the diffs, like the names of files you touched and identifiers you added there. |
|
Please also add a test for this change, and organize into clear and coherent commits according to the Zulip style — those are our two general requirements for a PR. See the repo's README for details, and links to further details. Once those are met, this will be ready for a review. After another maintainer has reviewed it and you've revised to address their comments, I'll review the PR. |
Although i did write the whole description in a paragraph, I use LLM to fine tune my description into a formatted output like a pointer and reformat it output again myself. I have changed the description to a brief one. |
Alright Greg |
Added Gesture Detector over Column, Temporary set the onPressed of the Icon Button to null. Fixes: zulip#1808 home: Improve performace and fix bug. Replaced IconButton widgets to Icon improve significantly performace of switching screens and wrote test for the semantic labels for App Bar. Fixes: zulip#1879
55f7e12 to
1f1a1ee
Compare
|
@chrisbobbe This is the PR for which we discussed in the chats For Design reference i have attached the difference.
I have made the commits coherent as well as minimal as request by Greg. |
e8fc538 to
406a86f
Compare
lib/widgets/home.dart
Outdated
| // TODO(#535): Decide if we find it helpful to use something like | ||
| // [SemanticsProperties.namesRoute] to structure this UI better | ||
| // for screen-reader software. | ||
| Offstage(offstage: tab != _tab.value, child: body), | ||
| Offstage( | ||
| offstage: tab != _tab.value, | ||
| child: Semantics( | ||
| namesRoute: true, | ||
| child: body)) |
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.
It looks like you read the TODO and decided that SemanticsProperties.namesRoute is helpful. Could you explain what you considered when making that decision?
406a86f to
88de8b2
Compare
|
Thank you @chrisbobbe for pointing out the issue in sematics! I have updated the semantic using SematicServices.sendAnnouncement for better interoperability. I can provide the TalkBack working on android recording for confirmation. |
2d51fd6 to
adc6013
Compare
Hmm, in #1879 (comment) , I didn't intend to point out any issue. 🙂 I just meant to ask for information that could help us understand your proposal. Did you find some issue with |
Thank you @chrisbobbe for the review, I think the main reason is offstage do not allow the semantic to access its property for semantic widget to correctly get the label to speak out, I also tested it using TalkBack and the semantic widget even with a label was not able to make TalkBack trigger when the change in page body was detected. |
|
It sounds like you were expected to hear something from TalkBack when you changed tabs, and you didn't hear anything, so you assumed it wasn't a valid fix for the TODO. I these are the main requirements to resolve the TODO:
It's entirely possible that a If it gets at all complicated, let's not try to resolve the TODO in this PR, to keep it focused on #1857. |
adc6013 to
ed0b571
Compare
Before Implementing the Nav Bar Label
After Implementing the Nav Bar Label
Case of Text Overflow( Upto 2 Lines )
Thank you @chrisbobbe for your review. I feels like it better to concentrate on a single task right now (i.e the nav bar labelling). This solution also fixes the constant size of nav bar even when the system font size was increased which was a user dissatisfier, it would allow the uses to easily identify the corresponding page using the enlarged label fitted within nav bar. |
Screenshot Comparison
Video Recording of implementationvideo.mp4Possible enhancement: Adding a padding in the top which was not implemented as it was not described in Figma Design of the same. Thanks for the patience, the PR is ready for review! @chrisbobbe, the Ink splash effect on the button has been restored using material and inkwell combination which also includes the padding as per the Figma design. |
ef68b0c to
4499f3f
Compare
|
Thank you @chrisbobbe for the detailed feedback, the text font scaling has been handled using |
chrisbobbe
left a comment
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! Small comments below.
test/widgets/home_test.dart
Outdated
| await tester.tap(find.descendant( | ||
| of: find.byType(DecoratedBox), | ||
| matching: find.text('Channels'))); |
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.
Let's make this finder more specific. DecoratedBox widgets are pretty commonly used, so there's some risk that this finder will find one that has nothing to do with creating the visual appearance of the nav bar.
We can add the expectation that it has a top border of a certain color and no other borders. For example, at the top of this 'bottom nav navigation' group, we could define a helper like findInBottomNav:
final findBottomNavDecoratedBox = find.byWidgetPredicate((widget) {
if (widget is! DecoratedBox) return false;
final decoration = widget.decoration;
if (decoration is! BoxDecoration) return false;
final expectedBorderTop = BorderSide(color: Colors.black.withValues(alpha: 0.2));
return decoration.border == Border(top: expectedBorderTop);
});
Finder findInBottomNav(Finder finder) =>
find.descendant(of: findBottomNavDecoratedBox, matching: finder);and use it like this in the tests:
await tester.tap(findInBottomNav(find.text('Channels')));
test/widgets/home_test.dart
Outdated
| check(find.descendant( | ||
| of: find.byType(ZulipAppBar), | ||
| matching: find.text('Direct messages'))).findsOne(); |
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.
nit: We use two-space indents, not four
| await tester.pump(); | ||
|
|
||
| check(find.descendant( |
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.
nit: Remove this blank line, to group together the "tap, pump, check result" actions. Similarly below.
lib/widgets/home.dart
Outdated
| mainAxisSize: MainAxisSize.min, | ||
| children: [ | ||
| SizedBox(height: 34, | ||
| child: Center( |
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.
Bump on #1879 (comment)
|
Thank you @chrisbobbe for the review. The following changes have been implemented: Reordered tests to verify internal consistency (state), then correct UI reflection (title), and finally user interaction (taps)Inside the Order of test before
Order of test after
This was done to ensure that appbar title are switched only when views are changed since if Also:
|
dcdbeee to
1342b79
Compare
|
@gnprice Thank you for updating regarding the new issue filed which can be easily solve within this. After trying for few hours, I would like to point out few things regarding the testing of TalkBack. From the trials i have done. The TalkBack was being displayed neither by Live Transcript not via any Screen Recording software which has support to record android internal audio. Hence i think its not possible to able to capture it and display it over here. However, I have tested this on my android device and the changes works as expected. The only way I think it can be shown is by setting up this branch in the local environment to test the talk back or using an external device(microphone) near the speaker to recording the TalkBack. Would it be helpful if I post the latter one result? |
8a6f94f to
07695e4
Compare
chrisbobbe
left a comment
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! Comments below.
lib/widgets/home.dart
Outdated
| Icon(icon, size: 24, color: color), | ||
| Flexible( | ||
| child: Text( | ||
| semanticsLabel: '$label tab', |
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.
From the doc for Text.semanticsLabel:
/// An alternative semantics label for this text.
///
/// If present, the semantics of this widget will contain this value instead
/// of the actual text. This will overwrite any of the semantics labels applied
/// directly to the [TextSpan]s.
///
/// This is useful for replacing abbreviations or shorthands with the full
/// text value:
///
/// ```dart
/// const Text(r'$$', semanticsLabel: 'Double dollars')
/// ```In this case the actual text is the value of label (e.g. "Combined feed" or "綜合饋給"), so if we didn't pass semanticsLabel, then that would be the semantics label too. If we pass semanticsLabel: '$label tab', as you're proposing here, then that would be the semantics label (e.g. "Combined feed tab" or "綜合饋給 tab").
We don't merge user-facing text that's not set up for translation, so if we wanted versions of these names that include "tab", we'd need to add them to assets/l10n/app_en.arb and read them via ZulipLocalizations.
Instead of doing that, though, let's use Flutter's documented API for declaring (to screen-reader software) than an element is a tab. See our existing use of SemanticsRole.tab and its documentation.
test/widgets/home_test.dart
Outdated
| check(find.descendant( | ||
| of: find.byType(ZulipAppBar), | ||
| matching: find.text('Inbox'))).findsOne(); |
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.
We use two-space indent, not four; I've already pointed this out: #1879 (comment)
c6aa556 to
8f15233
Compare
|
Thank you @chrisbobbe for the review. Have cleared out some nits as well as given |
|
Thanks! I've tested that out with VoiceOver and found a way to improve the experience. See the two commits I've pushed on top of yours; here's the list of all three: 8f15233 home: Add label to bottom navbar icons I'll mark this for Greg's review. If he requests changes, please pull the branch including my commits so we don't lose them, before applying your changes to the branch. |
gnprice
left a comment
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 @MritunjayTiwari14 for taking care of this, and @chrisbobbe for the previous reviews! Comments below.
lib/widgets/home.dart
Outdated
| child: ConstrainedBox( | ||
| // TODO(design): determine a suitable max width for bottom nav bar | ||
| constraints: const BoxConstraints(maxWidth: 600, minHeight: 48), |
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.
Does this maxWidth still work without the Center widget? What happens when the screen is wider than 600px?
lib/widgets/home.dart
Outdated
| return _NavigationBarButton(icon: icon, | ||
| selected: tabNotifier.value == tab, | ||
| onPressed: () { | ||
| tabNotifier.value = tab; | ||
| }, | ||
| label: label); |
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.
nit: keep label next to icon (here and below) — they're logically closely related, as the two pieces of visible content
lib/widgets/home.dart
Outdated
| _button(_HomePageTab.directMessages, ZulipIcons.two_person, | ||
| zulipLocalizations.recentDmConversationsPageTitle), | ||
| _NavigationBarButton( icon: ZulipIcons.menu, |
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.
I think this tabular-style format no longer really works now that there's the label making the _button call overflow onto two lines.
Instead, let's switch to a more standard format; and let's make icon and label be named parameters to _button, in order to be consistent with the direct _NavigationBarButton calls:
| _button(_HomePageTab.directMessages, ZulipIcons.two_person, | |
| zulipLocalizations.recentDmConversationsPageTitle), | |
| _NavigationBarButton( icon: ZulipIcons.menu, | |
| _button(_HomePageTab.directMessages, | |
| icon: ZulipIcons.two_person, | |
| label: zulipLocalizations.recentDmConversationsPageTitle), | |
| _NavigationBarButton( | |
| icon: ZulipIcons.menu, | |
| label: zulipLocalizations.navBarMenuLabel, |
| return Scaffold( | ||
| appBar: ZulipAppBar(titleSpacing: 16, | ||
| title: Semantics( | ||
| identifier: HomePage.titleSemanticsIdentifier, | ||
| namesRoute: true, | ||
| child: Text(_currentTabTitle))), | ||
| body: Semantics( | ||
| role: SemanticsRole.tabPanel, | ||
| identifier: HomePage.contentSemanticsIdentifier, | ||
| container: true, | ||
| explicitChildNodes: true, | ||
| child: Stack( |
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.
Hmm, interesting.
The rest is just a best effort at following the docs, which doesn't
make the experience obviously worse, but I didn't notice an effect
in my testing.
@chrisbobbe Were there particular docs you found especially helpful which you can link to? I'm curious for things we can consult in the future. (No need to spend a lot of time digging now, though — if things seem to behave well, that's good enough for the code at this stage.)
lib/widgets/home.dart
Outdated
| ]), | ||
| ), |
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.
nit:
| ]), | |
| ), | |
| ])), |
test/widgets/home_test.dart
Outdated
| final expectedBorderTop = BorderSide(color: Colors.black.withValues(alpha: 0.2)); | ||
| return decoration.border == Border(top: expectedBorderTop); | ||
| }); | ||
|
|
||
| // Finds a widget within the bottom navbar's decorated box subtree. | ||
| Finder findInBottomNav(Finder finder) => | ||
| find.descendant(of: findBottomNavDecoratedBox, matching: finder); |
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.
This seems fairly brittle. (In fact I believe it already wouldn't work in dark theme.) Conversely, I agree with @chrisbobbe's comment at #1879 (comment) above that looking for just DecoratedBox is too broad.
How about finding by SemanticsRole.tab? Or SemanticsRole.tabBar.
|
Thank you @gnprice for the review and @chrisbobbe for the commits. I have previously tried to use
|
|
Will push the updates by tomorrow after resolving some conflicts to clean commit history. |
28ad215 to
15ed7bd
Compare
Fixes zulip#1857. Fixes zulip#1960. Some of this has an effect I observes in my testing on iOS with VoiceOver: - MergeSemantics on each tab causes the tab to be represented by a single node instead of multiple, so it isn't double-visited when swiping right to traverse the tree. The rest is just a best effort at following the docs, which doesn't make the experience obviously worse, but I didn't notice an effect in my testing.
15ed7bd to
96cefc2
Compare






























Key Changes: (Screenshots Outdated)
I have kept the height of the Navigation to be fixed because a fully dynamic navigation bar height that adjusts based on label content across languages is not feasibly after having done some feasibility study. The UI jitter when the bar changes its size suddenly when the user changes the app language is extremely unlikely to be solved and can negatively impact user experience hence even in day to day app these types of dynamic navigation bar are not used.
integration of the page bodies and semantics has been done
Link to Figma Design was not found rather, the screenshot that was shared by Vlad Korobov in the following discuss was used to implement the UI : #mobile > Flutter feedback - Icons in app @ 💬
Thank you
Figma Design Link
Fixes #1857
Fixes #1960