Skip to content

Conversation

@1akhanBaheti
Copy link
Contributor

Feature Preview

image

FR raised here also :
AppFlowy-IO/AppFlowy#3481

@CLAassistant
Copy link

CLAassistant commented Sep 23, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@MayurSMahajan MayurSMahajan left a comment

Choose a reason for hiding this comment

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

Can you add a test for this, so we ensure new changes don't break your feature.

@1akhanBaheti
Copy link
Contributor Author

1akhanBaheti commented Sep 24, 2023

ok @MayurSMahajan, I will write test also for this.
but i have one query, that whether i should write test for toolbar widget OR computeActiveItems() function to check whether it generating toolbarItems with correct order of ToolbarItemID or not.

I have added the testcases for toolbar layout, please can you check once ?

@LucasXu0
Copy link
Collaborator

Nice try!

@1akhanBaheti
Copy link
Contributor Author

There was commit lint issue, that's why jobs were failing so,
i have refactored the commit message, any other changes required ? @MayurSMahajan

@MayurSMahajan
Copy link
Contributor

Looks good to me.

Comment on lines 182 to 204
List<ToolbarItem> _computeActiveItems() {
List<ToolbarItem> activeItems = widget.items
.where((e) => e.isActive?.call(editorState) ?? false)
.toList();
if (activeItems.isEmpty) {
return [];
}
if (widget.layoutDirection == TextDirection.rtl) {
activeItems = activeItems.reversed.toList();
}
// sort by group.
activeItems.sort(
(a, b) => widget.layoutDirection == TextDirection.ltr
? a.group.compareTo(b.group)
: b.group.compareTo(a.group),
);
// insert the divider.
return activeItems
.splitBetween((first, second) => first.group != second.group)
.expand((element) => [...element, placeholderItem])
.toList()
..removeLast();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you move this code from floating_toolbar_widget to here?

Copy link
Contributor Author

@1akhanBaheti 1akhanBaheti Sep 25, 2023

Choose a reason for hiding this comment

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

@LucasXu0
actually in tests, I was not able to verify the order of toolbarItems, that's why moved there so now I am able to verify it by taking it from parameters.
as IDs are assigned to ToolbarItem model, not to widgets.

@LucasXu0
Copy link
Collaborator

There was commit lint issue, that's why jobs were failing so, i have refactored the commit message, any other changes required ? @MayurSMahajan

yes. so please run flutter analyze, dart format ., and 'flutter test' before submitting your code.

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (8e61846) 80.48% compared to head (df30968) 80.58%.
Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #496      +/-   ##
==========================================
+ Coverage   80.48%   80.58%   +0.09%     
==========================================
  Files         281      280       -1     
  Lines       11800    11809       +9     
==========================================
+ Hits         9497     9516      +19     
+ Misses       2303     2293      -10     
Files Coverage Δ
...b/src/editor/toolbar/desktop/floating_toolbar.dart 83.67% <100.00%> (+5.32%) ⬆️
...ditor/toolbar/desktop/floating_toolbar_widget.dart 96.87% <100.00%> (+0.20%) ⬆️

... and 15 files with indirect coverage changes

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

@LucasXu0
Copy link
Collaborator

LucasXu0 commented Sep 27, 2023

@1akhanBaheti I optimized some parts of your code. please check this commit.

the most important change is that I used the text direction of the row instead of reversing the toolbar items.

@LucasXu0 LucasXu0 merged commit 2b58d4f into AppFlowy-IO:main Sep 27, 2023
q200892907 added a commit to q200892907/appflowy-editor that referenced this pull request Sep 27, 2023
* main: (39 commits)
  feat: support RTL in toolbar (AppFlowy-IO#496)
  fix: unable to update selection sometimes when the editor lost focus (AppFlowy-IO#509)
  fix: delete the divider on mobile will raise an error (AppFlowy-IO#508)
  fix: the cursor will flicker one frame to its previous position (AppFlowy-IO#506)
  fix: undo failed in a nested list in a special case (AppFlowy-IO#503)
  chore: bump version 1.4.3 (AppFlowy-IO#502)
  fix: image block rect overflow
  fix: the text within the <mark> tag didn't parse correctly when pasting HTML.
  chore: bump version 1.4.2
  fix: command shortcuts on the web platform should also respect the operating system
  fix: focus will lost when typing slash
  fix: unable to input text on web platform
  fix: unable to change RTL of heading (AppFlowy-IO#495)
  fix: fix mobile selection drag (AppFlowy-IO#490)
  chore: bump version 1.4.1
  fix: the placeholder doesn't appear on desktop platform
  fix: required named parameter 'onLiveTextInput' must be provided
  feat: add support for font family and font size attributes in rich text (AppFlowy-IO#486)
  fix: the color of text and highlight icon should be white (AppFlowy-IO#485)
  chore: bump version 1.4.0 (AppFlowy-IO#484)
  ...
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.

4 participants