Skip to content
This repository has been archived by the owner on Jun 13, 2024. It is now read-only.

Migrate all SelectableText to SelectionArea. #699

Closed
wants to merge 15 commits into from

Conversation

olegzaim
Copy link

@olegzaim olegzaim commented May 26, 2022

Replaced all SelectableText to SelectionArea.
Fixes #698

Copy link
Member

@guidezpl guidezpl left a 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 SelectionAreas 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.

@olegzaim olegzaim requested a review from guidezpl May 30, 2022 06:40
Copy link
Member

@guidezpl guidezpl left a 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

lib/studies/fortnightly/app.dart Outdated Show resolved Hide resolved
lib/studies/shrine/app.dart Outdated Show resolved Hide resolved
lib/studies/shrine/app.dart Outdated Show resolved Hide resolved
lib/studies/starter/app.dart Outdated Show resolved Hide resolved
@olegzaim
Copy link
Author

  • Rally text selection is invisible.

Hello, there is something wrong with the cursor for selection area because when I select all area it isn't invisible, but when I just tap and press it is stay invisible:

  • Single select with long press
    long press for select
  • Select all
    select all
  • Demos don't have text selection

Is it enough or it's need to be selectable also inside demos?
home and demos select

@guidezpl
Copy link
Member

I made some small fixes for consistency. The only thing missing I think is adding selection for the demos (material, cupertino, etc.)

@olegzaim
Copy link
Author

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 :)

@olegzaim olegzaim requested a review from guidezpl June 3, 2022 07:18
Copy link
Member

@guidezpl guidezpl left a comment

Choose a reason for hiding this comment

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

Thanks for contributing!

@guidezpl
Copy link
Member

guidezpl commented Jun 8, 2022

@chunhtai This change triggers errors that cause benchmark and golden tests to fail, would appreciate you taking a look if possible.

@guidezpl
Copy link
Member

@olegzaim Could you take a look at the merge conflicts so that we can land this? Thanks!

@guidezpl
Copy link
Member

Realized you might only have access to @OlegZaimNLT, if so, please create a new PR cloning this branch

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants