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.

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

levkropp
Copy link
Contributor

@levkropp levkropp commented Dec 18, 2024

closes #3821

This PR changes most Text widgets in the Instances and VM Details pages to SelectableText
SelectableText does not support overflow: ellipsis so I have set maxLines: 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

@levkropp levkropp linked an issue Dec 18, 2024 that may be closed by this pull request
Copy link

codecov bot commented Dec 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.02%. Comparing base (eeed24d) to head (0d04f37).
Report is 2 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

@levkropp levkropp changed the title change Text to SelectableText in GUI for instance info use SelectableText in GUI for instance info Dec 18, 2024
@levkropp levkropp force-pushed the gui-make-text-selectable branch from 251cb44 to 1719d61 Compare December 18, 2024 23:16
src/client/gui/lib/extensions.dart Outdated Show resolved Hide resolved
@@ -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 {
Copy link
Contributor

@andrei-toterman andrei-toterman Dec 19, 2024

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.

Copy link
Contributor Author

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 use import '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

Copy link
Contributor

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.

Copy link
Contributor

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!

@levkropp levkropp force-pushed the gui-make-text-selectable branch 3 times, most recently from 251195c to 4ba5a94 Compare December 19, 2024 20:14
@levkropp levkropp force-pushed the gui-make-text-selectable branch from 4ba5a94 to 0d04f37 Compare December 20, 2024 19:12
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.

Make text selectable
2 participants