-
Notifications
You must be signed in to change notification settings - Fork 656
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
use SelectableText in GUI for instance info #3852
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3852 +/- ##
==========================================
- Coverage 89.02% 89.02% -0.01%
==========================================
Files 255 255
Lines 14578 14577 -1
==========================================
- Hits 12978 12977 -1
Misses 1600 1600 ☔ View full report in Codecov by Sentry. |
251cb44
to
1719d61
Compare
src/client/gui/lib/extensions.dart
Outdated
@@ -107,3 +107,45 @@ extension NullableMap<T> on T? { | |||
} | |||
} | |||
} | |||
|
|||
/// A custom wrapper for SelectableText with a white popup (right-click) menu. | |||
class WhitePopupMenuSelectableText extends StatelessWidget { |
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.
That's kind of a lengthy name IMO. You can name it just SelectableText
and use import 'package:flutter/material.dart' hide SelectableText;
so that you can use yours without interfering with Flutter's.
Also, you can move this to its own file. extensions.dart
is more for defining useful extension methods on types rather than for widgets.
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.
You can name it just
SelectableText
and useimport 'package:flutter/material.dart' hide SelectableText;
so that you can use yours without interfering with Flutter's.
When I tried to do this I wasn't able to define the contextMenuBuilder parameter: The contextMenuBuilder parameter is not defined in the SelectableText widget.
So instead I moved the class into its own file at selectable_text.dart and renamed it to WhiteSelectableText
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.
Yeah, it was because your SelectableText
was conflicting with Flutter's here. You can circumvent this by importing Flutter under an alias in just this file. Take a look here where there is a custom Tooltip
widget that wraps Flutter's Tooltip
.
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 understand if this feels weird, and maybe it creates confusion. But IMO it's better than coming up with more elaborate names for widgets just because Flutter used them first. I'm open to criticism though!
251195c
to
4ba5a94
Compare
4ba5a94
to
0d04f37
Compare
closes #3821
This PR changes most
Text
widgets in the Instances and VM Details pages toSelectableText
SelectableText
does not supportoverflow: ellipsis
so I have setmaxLines: 1
to ensure that table cells are still 1 line max.I did not make the instance's name selectable in the Instances page because the name column has unique behavior implemented with
VMNameLink
where clicking it links to the instance's shell