-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Migrate all SelectableText to SelectionArea. #699
Conversation
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.
This is not quite what is required. Only ~5 SelectionArea
s should be necessary, one per MaterialApp
, and one for the setting page, each placed near the top of the widget tree.
Each SelectableText
should be replaced by Text
.
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.
To fix
- Rally text selection is invisible
- Home page doesn't have text selection
- Demos don't have text selection
…the Srine and Start apps.
I made some small fixes for consistency. The only thing missing I think is adding selection for the demos (material, cupertino, etc.) |
Okay, great thank you :) |
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.
Thanks for contributing!
@chunhtai This change triggers errors that cause benchmark and golden tests to fail, would appreciate you taking a look if possible. |
@olegzaim Could you take a look at the merge conflicts so that we can land this? Thanks! |
Realized you might only have access to @OlegZaimNLT, if so, please create a new PR cloning this branch |
Replaced all SelectableText to SelectionArea.
Fixes #698