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.

Sign up for GitHub

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

Jump to bottom

Show context menu on desktop #179

Merged
merged 7 commits into from
Dec 7, 2023
Merged

Show context menu on desktop #179

merged 7 commits into from
Dec 7, 2023

Conversation

Amir-P
Copy link
Member

@Amir-P Amir-P commented Dec 1, 2023

No description provided.

@codecov Codecov
Copy link

codecov bot commented Dec 3, 2023 •
edited
Loading

Codecov Report

Attention: 179 lines in your changes are missing coverage. Please review.

Comparison is base (97fbd27) 84.25% compared to head (99bb395) 84.55%.
Report is 2 commits behind head on master.

Files Patch % Lines
...kages/fleather/lib/src/widgets/text_selection.dart 56.90% 178 Missing ⚠️
packages/fleather/lib/src/widgets/editor.dart 95.83% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #179      +/-   ##
==========================================
+ Coverage   84.25%   84.55%   +0.30%     
==========================================
  Files          60       60              
  Lines        9408     9617     +209     
==========================================
+ Hits         7927     8132     +205     
- Misses       1481     1485       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Amir-P Amir-P mentioned this pull request Dec 4, 2023
Open
5 tasks
@Amir-P Amir-P requested a review from amantoux December 4, 2023 09:48
@Amir-P Amir-P marked this pull request as ready for review December 4, 2023 09:48
amantoux
amantoux reviewed Dec 6, 2023
View reviewed changes
Copy link
Member

@amantoux amantoux left a comment

Choose a reason for hiding this comment

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

Great to see this feature!
Some comments & questions, but overall ok

packages/fleather/lib/src/rendering/editor.dart Outdated Show resolved Hide resolved
packages/fleather/lib/src/rendering/editor.dart Outdated Show resolved Hide resolved
packages/fleather/lib/src/widgets/text_selection.dart Outdated Show resolved Hide resolved
packages/fleather/lib/src/widgets/text_selection.dart Show resolved Hide resolved
packages/fleather/lib/src/widgets/text_selection.dart Show resolved Hide resolved
packages/fleather/lib/src/widgets/text_selection.dart Show resolved Hide resolved
@amantoux
Copy link
Member

amantoux commented Dec 6, 2023

@Amir-P Did you run some manual tests on iOS and Android? The changes impact it quite a lot

@Amir-P
Copy link
Member Author

Amir-P commented Dec 6, 2023 •
edited
Loading

@Amir-P Did you run some manual tests on iOS and Android? The changes impact it quite a lot

I only got to test it on mac and iOS emulator. Haven't tested it on Android and Web yet. I also don't have access to Linux and Windows machine. This PR has so many potential to introduce unintended behaviors but as you know, it is copied from Flutter's source code with very little modifications. @amantoux

@amantoux
Copy link
Member

amantoux commented Dec 6, 2023

@Amir-P Did you run some manual tests on iOS and Android? The changes impact it quite a lot

I only got to test it on mac and iOS emulator. Haven't tested it on Android and Web yet. I also don't have access to Linux and Windows machine. This PR has so many potential to introduce unintended behaviors but as you know, it is copied from Flutter's source code with very little modifications. @amantoux

Ok never mind
We see that in another PR if need be

@Amir-P Amir-P requested a review from amantoux December 6, 2023 19:35
amantoux
amantoux approved these changes Dec 7, 2023
View reviewed changes
Copy link
Member

@amantoux amantoux left a comment

Choose a reason for hiding this comment

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

LGTM

@amantoux amantoux merged commit 6ba157f into master Dec 7, 2023
3 of 4 checks passed
@Amir-P Amir-P deleted the feat/desktop_context_menu branch December 7, 2023 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amantoux amantoux amantoux approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Amir-P @amantoux