Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Load a snapshot. #6974

Merged
merged 41 commits into from
Jan 31, 2024
Merged

Load a snapshot. #6974

merged 41 commits into from
Jan 31, 2024

Conversation

polina-c
Copy link
Contributor

@polina-c polina-c commented Dec 15, 2023 •
edited
Loading

Contributes to #5606

For release notes:

Screenshot 2024-01-30 at 11 26 07 AM Screenshot 2024-01-25 at 8 09 36 PM
Screen.Recording.2024-01-25.at.4.08.56.PM.mov

@polina-c polina-c requested review from bkonyi and a team as code owners December 15, 2023 01:10
@polina-c polina-c changed the title Lead a snapshot. Load a snapshot. Dec 15, 2023
@polina-c
Copy link
Contributor Author

TODO: add test coverage

@kenzieschmoll
Copy link
Member

DBC, I'd align the upload icon to the far right of the controls row instead
Screenshot 2024-01-29 at 9 54 58 AM

@kenzieschmoll
Copy link
Member

Another observation from your demo: we should fix whatever this overlapping / white space issue is
Screenshot 2024-01-29 at 9 56 53 AM

@polina-c
Copy link
Contributor Author

Another observation from your demo: we should fix whatever this overlapping / white space issue is Screenshot 2024-01-29 at 9 56 53 AM

Elipsis is not available yet. Upvoted bug and added TODO.

kenzieschmoll
kenzieschmoll reviewed Jan 30, 2024
View reviewed changes
packages/devtools_app/lib/src/screens/memory/panes/diff/widgets/snapshot_list.dart
Comment on lines +84 to +88
ToolbarAction(
icon: Icons.file_upload,
tooltip: 'Import snapshot(s) from disk',
onPressed: () => unawaited(controller.importSnapshots()),
),
Copy link
Member

Choose a reason for hiding this comment

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

we should move this to be the last item in the row, aligned to the right. Consistent with other parts of DevTools that have an import action as a right aligned control.

Copy link
Member

Choose a reason for hiding this comment

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

Or at least have a separator like Chrome DevTools does if we want to keep the controls left aligned:
Screenshot 2024-01-30 at 8 54 58 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2024-01-30 at 11 26 07 AM

@polina-c polina-c added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 31, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 31, 2024
@auto-submit Auto-submit
Copy link

auto-submit bot commented Jan 31, 2024

auto label is removed for flutter/devtools/6974, due to - The status or check suite macos goldens test_dart2js has failed. Please fix the issues identified (or deflake) before re-applying this label.

@polina-c polina-c merged commit 90687d2 into flutter:master Jan 31, 2024
23 checks passed
@polina-c polina-c deleted the load branch January 31, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kenzieschmoll kenzieschmoll kenzieschmoll approved these changes

@bkonyi bkonyi Awaiting requested review from bkonyi bkonyi is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@polina-c @kenzieschmoll