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

Cannot make toolbar appear on empty document (immediately after gaining focus) #28

Merged
merged 7 commits into from Aug 31, 2022

Conversation

amantoux
Copy link
Member

@amantoux amantoux commented Aug 25, 2022

Fixes #29


Before fix (go to menu once document is open, emulates opening an empty document and setting focus)

Before.fix.mov

After fix

After.fix.mov

@codecov
Copy link

codecov bot commented Aug 25, 2022

Codecov Report

Merging #28 (bb874b8) into master (5da5873) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master      #28      +/-   ##
==========================================
- Coverage   44.31%   44.29%   -0.02%     
==========================================
  Files          30       30              
  Lines        4044     4041       -3     
==========================================
- Hits         1792     1790       -2     
+ Misses       2252     2251       -1     
Impacted Files Coverage Δ
packages/fleather/lib/src/rendering/editor.dart 46.44% <50.00%> (-0.16%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@fleather-editor fleather-editor locked and limited conversation to collaborators Aug 25, 2022
@amantoux amantoux closed this Aug 25, 2022
@amantoux amantoux reopened this Aug 25, 2022
@amantoux amantoux marked this pull request as draft August 25, 2022 13:56
@amantoux amantoux force-pushed the toolbar_not_appear_on_empty_document branch from bb874b8 to 6e347a5 Compare August 25, 2022 17:40
@amantoux amantoux marked this pull request as ready for review August 25, 2022 18:16
@amantoux amantoux requested a review from Amir-P August 25, 2022 18:16
Copy link
Member

@Amir-P Amir-P left a comment

Choose a reason for hiding this comment

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

LGTM!

@Amir-P
Copy link
Member

Amir-P commented Aug 30, 2022

Just a question: Why did you re-add removed TODOs? @amantoux

@amantoux
Copy link
Member Author

amantoux commented Aug 31, 2022

Just a question: Why did you re-add removed TODOs? @amantoux

Did I? last commit removes them I beleive

@amantoux
Copy link
Member Author

amantoux commented Aug 31, 2022

@Amir-P
I've added the same behavior with double tap

NB: We need to review the whole Editor renderer - widget in order to stick to Flutter latest Editable - EditableText arch; particularly with regards to focus management and user interactions

@amantoux amantoux requested a review from Amir-P August 31, 2022 06:59
@@ -608,7 +608,8 @@ class RenderEditor extends RenderEditableContainerBox
// as well.
if (selectionChanged ||
(cause == SelectionChangedCause.longPress ||
cause == SelectionChangedCause.keyboard)) {
Copy link
Member

@Amir-P Amir-P Aug 31, 2022

Choose a reason for hiding this comment

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

I think it's better to extract this into an extension method on SelectionChangedCause. WDYT? @amantoux

Copy link
Member Author

Choose a reason for hiding this comment

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

@Amir-P what do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

Something like this:

Extension on SelectionChangedCause {
  bool get shouldSendEvent => this == SelectionChangedCause.longPress || ...
}

WDYT? @amantoux

Copy link
Member Author

@amantoux amantoux Aug 31, 2022

Choose a reason for hiding this comment

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

Ok, I'm not sure it will be needed though it the future

Done; PTAL @Amir-P

@Amir-P Amir-P merged commit e5446f6 into master Aug 31, 2022
@amantoux amantoux deleted the toolbar_not_appear_on_empty_document branch September 1, 2022 18:23
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.

Toolbar doesn't appear after a long press while gaining focus
2 participants