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

fix: delete node does not propagate non null selection #45

Merged
merged 10 commits into from
Apr 11, 2023
Merged

fix: delete node does not propagate non null selection #45

merged 10 commits into from
Apr 11, 2023

Conversation

squidrye
Copy link
Contributor

@squidrye squidrye commented Apr 6, 2023

fixes #39

@CLAassistant
Copy link

CLAassistant commented Apr 6, 2023

CLA assistant check
All committers have signed the CLA.

@LucasXu0
Copy link
Collaborator

LucasXu0 commented Apr 7, 2023

Hey, @squidrye. Not every delete can assign the beforeSelection to the afterSelection. For example,

Before
Node 1
|Node 2| <- delete it. || means the selection

After
Node 1| <- now, the afterSelection is not equal to the beforeSelection

@squidrye
Copy link
Contributor Author

squidrye commented Apr 7, 2023

Thanks for pointing that out, I'll see what can be done.

@squidrye
Copy link
Contributor Author

squidrye commented Apr 7, 2023

I have added a condition to check if node being deleted is currently selected node, if so after-selection will not be updated, but if node being deleted is not selected and some node in background is selected it will be propagated and maintained.

|Node 1|
Node 2 <- deleted then Node 1 will be still selected

|Node 1| <- deleted then afterSelection will not be updated

Let me know if I am missing something else here.

@squidrye squidrye marked this pull request as draft April 7, 2023 09:40
@squidrye squidrye marked this pull request as ready for review April 8, 2023 20:44
@LucasXu0
Copy link
Collaborator

Hey, @squidrye. Please format your code.

@codecov
Copy link

codecov bot commented Apr 11, 2023

Codecov Report

Merging #45 (42ffa09) into main (645e430) will increase coverage by 0.62%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #45      +/-   ##
==========================================
+ Coverage   82.04%   82.66%   +0.62%     
==========================================
  Files         124      124              
  Lines        7179     7189      +10     
==========================================
+ Hits         5890     5943      +53     
+ Misses       1289     1246      -43     
Impacted Files Coverage Δ
lib/src/core/transform/transaction.dart 95.87% <100.00%> (+0.10%) ⬆️

... and 4 files with indirect coverage changes

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

@LucasXu0 LucasXu0 merged commit 8602a36 into AppFlowy-IO:main Apr 11, 2023
@annieappflowy annieappflowy added the bug Something isn't working label Apr 13, 2023
Xazin pushed a commit to Xazin/appflowy-editor that referenced this pull request Apr 13, 2023
* fix: delete node does not propagate non null selection

* test: delete node does not propagate non null selection

* refactor: lucas's suggestion

* refactor: suggestions

* refactor: revert inSelection, is not able to differentiate between overlays and selection.

* test: updated tests

* refactor: code formatting

* refactor: code formatting

* chore: dart format

---------

Co-authored-by: Lucas.Xu <lucas.xu@appflowy.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Bug] replace does not work in certain use-cases
4 participants