[flutter_adaptive_scaffold] : 🐛 [FIX] : Issue: 121135. #3253
Conversation
- CHANGELOG.md updated. - Version updated to 0.1.1. - ✅ : Test case updated as per changes in /example.
|
@gspencergoog PR updated. |
| @@ -1,3 +1,7 @@ | |||
| ## 0.1.1 | |||
|
|
|||
| * 🐛 : FIX - [**121135**](https://github.com/flutter/flutter/issues/121135) : `selectedIcon` parameter not displayed even if it is provided from example. | |||
There was a problem hiding this comment.
Let's stick with a more standard bug fix notice without the emoji.
| * 🐛 : FIX - [**121135**](https://github.com/flutter/flutter/issues/121135) : `selectedIcon` parameter not displayed even if it is provided from example. | |
| * Fixes flutter/flutter#121135) `selectedIcon` parameter not displayed even if it is provided |
Also, see the style guilde for this: https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changelog-style
There was a problem hiding this comment.
Thanks for sending the guide for change log updation. I will do the needful in my future commits.
packages/flutter_adaptive_scaffold/example/lib/adaptive_layout_demo.dart
Show resolved
Hide resolved
| @@ -35,46 +42,64 @@ class MyHomePage extends StatelessWidget { | |||
| Padding( | |||
| padding: const EdgeInsets.all(8.0), | |||
| child: Container( | |||
| color: const Color.fromARGB(255, 255, 201, 197), | |||
| color: const Color.fromARGB(255, 255, 29, 197), | |||
There was a problem hiding this comment.
Why this color change?
There was a problem hiding this comment.
It was an unintentional change. I can revert that.
|
@gspencergoog I have updated PR with changes made upon your comments. |
# Conflicts: # packages/flutter_adaptive_scaffold/CHANGELOG.md
There was a problem hiding this comment.
This is looking better!
We still need some more testing, though. Can you write some more tests that make sure that your changes won't get accidentally reverted in the future, and that your changes continue to work as intended? Typically we would update tests for the demos and the adaptive_scaffold itself.
| @@ -142,6 +153,10 @@ class MyHomePage extends StatelessWidget { | |||
| inAnimation: AdaptiveScaffold.leftOutIn, | |||
| key: const Key('Primary Navigation Medium'), | |||
| builder: (_) => AdaptiveScaffold.standardNavigationRail( | |||
| selectedIndex: selectedNavigation, | |||
| onDestinationSelected: (int newIndex) { | |||
| setState(() => selectedNavigation = newIndex); | |||
There was a problem hiding this comment.
We don't usually use inline functions for setState arguments. The convention is to put them in braces. (here, and elsewhere in the PR).
| return Builder( | ||
| builder: (_) { | ||
| return BottomNavigationBar( | ||
| currentIndex: currentIndex ?? 0, | ||
| iconSize: iconSize, | ||
| type: BottomNavigationBarType.fixed, |
There was a problem hiding this comment.
I might be wrong about this, but doesn't this disable the animation on the bottom navigation bar? Was there a reason you added this?
There was a problem hiding this comment.
I updated that.
|
Regarding to the widget testing of the component and demos, I have made some changes in my past commit addressing the widget related issue. Apart from that, I couldn't see any test that is missing. As we cant actually test the things with selectedIcon (the change we made) as it is an optional parameter so its upto the consumer, how he/she wants to utilise it. |
|
@gspencergoog, let me know your further concerns about it. |
| return NavigationRailDestination( | ||
| label: Text(destination.label), | ||
| icon: destination.icon, | ||
| selectedIcon: destination.selectedIcon, |
There was a problem hiding this comment.
In the test/adaptive_scaffold_test.dart file, you can create a new test to find this icon to make sure that it is being displayed. You might have to have the test tap on the button to select it. To find the icon's widget, you can use find.byIcon(), giving it the selected icon you designated in the test.
There was a problem hiding this comment.
Test cases added for determining selected Icon availability in UI if any tap gesture happens to navigation rail. Used find.byKey() for finding specific icons in UI.
… selected icon if given.
| destinations.length, | ||
| ); | ||
|
|
||
| for (final NavigationRailDestination element |
There was a problem hiding this comment.
tiny nit: "destination" might be a better name, since element is an overloaded term here.
There was a problem hiding this comment.
I Will make change in some time and update the PR.
There was a problem hiding this comment.
@gspencergoog : Test case updated.
gspencergoog
added
the
autosubmit
* 7636eb7 [go_router_builder] Support default value for `Set`, `List` and `Iterable` route parameters (flutter/packages#3391) * 26c95da [image_picker] Move HashSet construction within if-statement (flutter/packages#3448) * f5687b2 [image_picker] fix typos in comments (flutter/packages#3413) * 84afba7 [image_picker] Migrate Android to Pigeon (flutter/packages#3476) * 42927fc [image_picker]: Bump androidx.exifinterface:exifinterface from 1.3.3 to 1.3.6 in /packages/image_picker/image_picker_android/android (flutter/packages#3238) * 9a44bdf Require Dart SDK 2.14, because of using APIs. (flutter/packages#3468) * 12609a2 [ci] Clean up iOS simulators (flutter/packages#3458) * 9b136a9 [ci/tool] Add external dependency validation (flutter/packages#3466) * 11aab47 Manual roll Flutter from fb7e828 to 67e5f66 (8 revisions) (flutter/packages#3482) * 784291b Update goldens (flutter/packages#3442) * 43a42d1 [webview_flutter_android] Updates Dart and Java InstanceManagers (flutter/packages#3282) * d0de136 [camera] Reland android flip/change camera while recording (flutter/packages#3460) * 74fd094 [image_picker_android] Adjust file extension in FileUtils when it does not match its mime type (flutter/packages#3409) * d2f1d2d [flutter_adaptive_scaffold] : 🐛 [FIX] : Issue: 121135. (flutter/packages#3253) * 3d078b5 Roll Flutter from 67e5f66 to 53dfd23 (39 revisions) (flutter/packages#3484) * 3b3a09d [url_launcher] Convert iOS to Pigeon (flutter/packages#3481) * 80cd50a Roll Flutter from 53dfd23 to 6bd2b3c (17 revisions) (flutter/packages#3486) * 998bb29 [webview_flutter] Updates the README with the migration of `WebView.initialCookies` and Hybrid Composition on (flutter/packages#3470) * bbf86a7 Roll Flutter from 6bd2b3c to 3e4e092 (7 revisions) (flutter/packages#3490)
[flutter_adaptive_scaffold] : � [FIX] : Issue: 121135.
Changes included in PR are listed as follows
List which issues are fixed by this PR. You must list at least one issue.
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.