Skip to content

make the heap data table the default view#2058

Merged
devoncarew merged 1 commit intoflutter:masterfrom
devoncarew:switch_heap_map_default
Jun 9, 2020
Merged

make the heap data table the default view#2058
devoncarew merged 1 commit intoflutter:masterfrom
devoncarew:switch_heap_map_default

Conversation

@devoncarew
Copy link
Contributor

  • make the heap data table the default view
  • add an outline around the heap data table
  • use common constants for the search box sizes

This PR has a few UI related changes, but the main change is that it changes the default view for the heap snapshot from the heat map to the heap data table.

cc @terrylucas

@devoncarew devoncarew merged commit 2e188c4 into flutter:master Jun 9, 2020

ValueListenable<bool> get showHeatMap => _showHeatMap;

void toggleShowHeatMap(bool value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just expose showHeatMap as a ValueNotifier if we are fine with users reading and writing the value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do; this pattern does map a little closer to how we do it on other controllers. I don't have a strong opinion -

const Text('Heat Map'),
Switch(
value: controller.showHeatMap,
value: controller.showHeatMap.value,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing where we are listening for changes to controller.showHeatMap to update this UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not; this page generally just calls setState() for most changes and rebuilds the page. We may want to increase the granularity of the changes were listening to here. This PR really just changes the default Switch value w/o making a lot of other changes to the existing code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants