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

feat: Multi list indentation at once #903

Merged
merged 9 commits into from
Oct 8, 2024

Conversation

Ahad-patel
Copy link
Contributor

Closes : AppFlowy-IO/AppFlowy#6255

This PR solves the issue of mulitple indentation of array lists at once.

Screen.Recording.2024-09-26.at.4.51.40.PM.mov

@CLAassistant
Copy link

CLAassistant commented Sep 26, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

codecov bot commented Sep 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.06%. Comparing base (54edcad) to head (e207b5f).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #903      +/-   ##
==========================================
+ Coverage   71.89%   72.06%   +0.17%     
==========================================
  Files         319      318       -1     
  Lines       14959    14986      +27     
==========================================
+ Hits        10755    10800      +45     
+ Misses       4204     4186      -18     
Flag Coverage Δ
?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@Ahad-patel
Copy link
Contributor Author

Hey, @Xazin @LucasXu0

I have added the feature for mulit list indentation at once, some of the tests are failing can you help me solve these issues?

@LucasXu0
Copy link
Collaborator

LucasXu0 commented Sep 30, 2024

@Ahad-patel There's one more case that should be considered.

The getNodesInSelection function will return all the nodes, including the children. For example, the tab command doesn't work in the case shown in the attached screenshot. When judging if the nodes are indentable, we just need to consider the first level and filter out their children nodes.

And please add more tests to cover this case.

Screenshot 2024-09-30 at 10 23 08

Update: I added a commit: 91753eb

@Ahad-patel
Copy link
Contributor Author

@LucasXu0 ok i'll fix the outdent command then will create test cases

@Ahad-patel
Copy link
Contributor Author

Hey @LucasXu0,

I have updated outdent_command file according to your changes, I have also added tests cases for indent_command and outdent_command.

I noticed that the outdent_command test file was named "outdent_handler_test.dart" ,since other test files were named according to their file name, so I renamed it to "outdent_command_test.dart".

I also removed isAllOnSameLevel because we are ignoring child nodes, so the isAllOnSameLevel method will always be true.

Are there any more remaining tasks in this PR?

@LucasXu0
Copy link
Collaborator

LucasXu0 commented Oct 1, 2024

I noticed that the outdent_command test file was named "outdent_handler_test.dart" ,since other test files were named according to their file name, so I renamed it to "outdent_command_test.dart".

Good catch.

Are there any more remaining tasks in this PR?

I'm checking the newly added code and will comment on the PR.

(tester) async {
const text = 'Welcome to Appflowy 😁';
final editor = tester.editor
// final editor = tester.editor..addNode(paraNode);
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

(tester) async {
const text = 'Welcome to Appflowy 😁';
final editor = tester.editor
// final editor = tester.editor..addNode(paraNode);
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@Ahad-patel
Copy link
Contributor Author

Hey @LucasXu0, I have updated the changes you suggested.

While testing I also noted one more thing, but I am unsure if it is a bug or it is working as intended. There is a selection before and after the line when this is in selection editorState.getNodesInSelection will give both the nodes.

Screen.Recording.2024-10-03.at.1.52.08.PM.mov

@LucasXu0
Copy link
Collaborator

LucasXu0 commented Oct 4, 2024

There is a selection before and after the line when this is in selection editorState.getNodesInSelection will give both the nodes.

It works as intended. You can see the toolbar location. Actually, the previous line has been selected as well. An improvement we can make here is to add a visual selection area for the previous line.

Screenshot 2024-10-04 at 11 10 12

@Ahad-patel
Copy link
Contributor Author

An improvement we can make here is to add a visual selection area for the previous line.

Should I make an new Issue and new PR for this ?

@Xazin
Copy link
Contributor

Xazin commented Oct 6, 2024

There is a selection before and after the line when this is in selection editorState.getNodesInSelection will give both the nodes.

It works as intended. You can see the toolbar location. Actually, the previous line has been selected as well. An improvement we can make here is to add a visual selection area for the previous line.

Normally if you have the cursor before the last character, and you use Shift + arrow right, it'll select the last character without the newline (so not the next line). This should be seen as a bug.

@Ahad-patel
Copy link
Contributor Author

Normally if you have the cursor before the last character, and you use Shift + arrow right, it'll select the last character without the newline (so not the next line). This should be seen as a bug.

I have created a new issue for this AppFlowy-IO/AppFlowy#6500

@LucasXu0 LucasXu0 merged commit 647a53e into AppFlowy-IO:main Oct 8, 2024
6 of 7 checks passed
@Ahad-patel Ahad-patel deleted the multi-list-indentation branch October 10, 2024 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] Multiple list indentation at one
5 participants