-
Notifications
You must be signed in to change notification settings - Fork 876
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
Fix selection in readOnly mode, Add magnifier via RawMagnifier widget #2529
base: master
Are you sure you want to change the base?
Conversation
From what I see, this restores the magnifier behavior, right? We've had issues with this implementation before, and it led to some problems during rollbacks. For the reasons mentioned above, it's best to test this PR until we know it won't cause us problems, like it did last time. In any case, thank you for taking the time and effort to make these changes. |
This does restore magnifier behavior. I did not create the previous magnifier implementation, but I did fix some of it's bugs that had been merged. However, this is a completely different implementation that is using Flutter's RawMagnifier widget in the Widget tree (see text_selection.dart) instead of changing events / config / etc throughout the editor. The other dart changes in this PR are just passing a ValueNotifier around, and that ValueNotifier is only used in text_selection.dart. (This PR also has some selection fixes when in read only mode) I agree needs to be tested. I have an internal team of 8, 1.2 million app users creating just over 10K notes a day. They have been running the 10.8.5 version that had my previous magnifier fixes. Prior to the fixes, we received several support tickets about the magnifier bugs, since the fixes no reports. After we finish internal testing, we will do a rollout to our users and the feature will be used there. We have finished updating to flutter_quill 11.x and require the selection and magnification changes. |
…llMangifierBuilder, also added a defaultQuillMagnifierBuilder function, when not specified the magnifier will not be used.
As a minor note, we should update the v11 migration guide to inform clients that the magnifier has been restored in version X after it was removed. This is not part of this PR since it should be done after releasing the new version. |
Description
Related Issues
Type of Change