-
Notifications
You must be signed in to change notification settings - Fork 336
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
Add search to the Network profiler #2333
Conversation
kenzieschmoll
commented
Sep 11, 2020
@@ -161,6 +165,18 @@ class _NetworkScreenBodyState extends State<NetworkScreenBody> | |||
_networkController.clear(); | |||
}, | |||
), | |||
const Expanded(child: SizedBox()), |
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.
can you change the properties on the row instead of adding this degenerate expanded?
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.
There are six other items in the row, so changing the row alignment would affect all of those items as well. I could wrap all the other items in their own row, and then use spaceBetween
but this felt simpler and cleaner
final isPreviousActiveSearchMatch = activeSearchMatch == widget.node; | ||
final isNewActiveSearchMatch = | ||
widget.activeSearchMatchNotifier.value == widget.node; | ||
activeSearchMatch = widget.activeSearchMatchNotifier.value; |
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.
move this duplicated code into a helper.
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's not actually duplicated - its searchMatch vs activeSearchMatch, which are two different entities
@@ -740,6 +739,10 @@ class _TableState<T> extends State<_Table<T>> with AutoDisposeMixin { | |||
} | |||
}); | |||
}); | |||
|
|||
if (widget.activeSearchMatchNotifier != null) { |
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.
redundant check for the notifier being not null.
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.
removed now that the check is inside _initSearchListener
@@ -740,6 +739,10 @@ class _TableState<T> extends State<_Table<T>> with AutoDisposeMixin { | |||
} | |||
}); | |||
}); | |||
|
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.
why isn't the selection notifier listener also tracked in the helper called from initState and didUpdateWidget?
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 didn't modify anything about the selection listener because it's not in the scope of this PR - will add a note to address
Fixes #2316 |