feat: add outline block #2750

AmanNegi · 2023-06-09T13:46:46Z

Feature Preview

Fixes: #2688

table_of_contents.mp4

PR Checklist

My code adheres to the AppFlowy Style Guide
I've listed at least one issue that this PR fixes in the description above.
I've added a test(s) to validate changes in this PR, or this PR only contains semantic changes.
All existing tests are passing.

AmanNegi · 2023-06-09T13:55:49Z

@LucasXu0 I was able to achieve the following output so far. Some of the issues I am facing are:

Outline rebuilds at every transaction in the document.
Additional attributes don't show up as expected (eg: hyperlink in heading doesn't show up in outline).

I would love to know what you have to suggest for these issues. Also please let me know about your views on the progress so far.

LucasXu0 · 2023-06-10T06:43:10Z

Outline rebuilds at every transaction in the document.

You can filter the transaction. only rebuilding if the heading is changed.

Additional attributes don't show up as expected (eg: hyperlink in heading doesn't show up in outline).

I think we just need to display the heading without its inline styles, such as color and href.

LucasXu0 · 2023-06-10T06:43:53Z

...lutter/lib/plugins/document/presentation/editor_plugins/outline/outline_block_component.dart

+class OutlineBlockKeys {
+  const OutlineBlockKeys._();
+
+  static const String type = 'outline_block';


Suggested change

static const String type = 'outline_block';

static const String type = 'outline';

LucasXu0 · 2023-06-10T06:44:08Z

...lutter/lib/plugins/document/presentation/editor_plugins/outline/outline_block_component.dart

+// defining the callout block menu item for selection
+SelectionMenuItem outlineItem = SelectionMenuItem.node(
+  name: 'Outline',
+  iconData: Icons.clear_all,


I have selected a few icons which match our purpose here, let me know which feels the best selection here:

The list_alt looks better.

LucasXu0 · 2023-06-10T06:45:17Z

Let me checkout your branch and add the suggestion comment.

LucasXu0 · 2023-06-11T13:25:25Z

frontend/appflowy_flutter/lib/plugins/document/presentation/editor_page.dart

@@ -1,6 +1,7 @@
 import 'package:appflowy/plugins/document/application/doc_bloc.dart';
 import 'package:appflowy/plugins/document/presentation/editor_plugins/actions/option_action.dart';
 import 'package:appflowy/plugins/document/presentation/editor_plugins/actions/block_action_list.dart';
+import 'package:appflowy/plugins/document/presentation/editor_plugins/outline/outline_block_component.dart';


add the file path to frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/plugins.dart

LucasXu0 · 2023-06-11T13:27:08Z

...lutter/lib/plugins/document/presentation/editor_plugins/outline/outline_block_component.dart

+
+// defining the callout block menu item for selection
+SelectionMenuItem outlineItem = SelectionMenuItem.node(
+  name: 'Outline',


please use l10n instead. Adding 'outline' to en.json.

I checked out other plugins for reference but no one uses i10n for the SelectionMenuItem name. Let me know if the following approach would be fine:

en.json

"outline": { "name": "Outline", "heading": "Table of Contents" }

Yep. The l10n for other selection menu items are missing. You can add the new l10n to ↓

LucasXu0 · 2023-06-11T13:31:53Z

...lutter/lib/plugins/document/presentation/editor_plugins/outline/outline_block_component.dart

+  }
+
+  @override
+  bool validate(Node node) => true;


validate the outline_block.children must be empty.

LucasXu0 · 2023-06-11T14:14:04Z

...lutter/lib/plugins/document/presentation/editor_plugins/outline/outline_block_component.dart

+        crossAxisAlignment: CrossAxisAlignment.start,
+        children: [
+          Text(
+            "TABLE OF CONTENTS: ",


Just for clarification would the following do fine?

"mathEquation": { "addMathEquation": "Add Math Equation", "editMathEquation": "Edit Math Equation" }, "outline":{ "heading": "TABLE OF CONTENTS:" },

Similar to the mathEquation plugin I also created the outline key.

LucasXu0 · 2023-06-11T14:14:55Z

...lutter/lib/plugins/document/presentation/editor_plugins/outline/outline_block_component.dart

+        children: [
+          Text(
+            "TABLE OF CONTENTS: ",
+            style: editorState.editorStyle.textStyleConfiguration.text,


Suggested change

style: editorState.editorStyle.textStyleConfiguration.text,

style: configuration.textStyle(node),

The configuration.textStyle(node) doesn't work as expected when the font size of the document is changed. However the earlier approach does work as expected.

You're right. we should use editorState.editorStyle.textStyleConfiguration.text here. I misread it.

This PR looks good to me, except for the missing test. I will merge it once you add the tests and ensure that all the CI passes.

LucasXu0 · 2023-06-11T14:15:57Z

@AmanNegi Please check the comments and add the tests.

codecov · 2023-06-11T14:31:56Z

Codecov Report

Merging #2750 (be490e3) into main (7f74fd6) will increase coverage by 54.34%.
The diff coverage is 9.41%.

@@             Coverage Diff             @@
##             main    #2750       +/-   ##
===========================================
+ Coverage   12.36%   66.70%   +54.34%     
===========================================
  Files         403      407        +4     
  Lines       19093    19333      +240     
===========================================
+ Hits         2360    12897    +10537     
+ Misses      16733     6436    -10297

Flag	Coverage Δ
appflowy_flutter_integrateion_test	`64.63% <9.41%> (?)`
appflowy_flutter_unit_test	`12.22% <0.00%> (-0.14%)`	⬇️

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

Impacted Files	Coverage Δ
...lugins/database/referenced_database_menu_item.dart	`100.00% <ø> (ø)`
...ditor_plugins/outline/outline_block_component.dart	`4.93% <4.93%> (ø)`
...lib/plugins/document/presentation/editor_page.dart	`96.12% <100.00%> (+96.12%)`	⬆️

... and 322 files with indirect coverage changes

Xazin · 2023-06-12T15:48:54Z

...lutter/lib/plugins/document/presentation/editor_plugins/outline/outline_block_component.dart

+  @override
+  Node get node => widget.node;
+
+  late EditorState editorState = context.read<EditorState>();


Late but you assign it a value immediately?

The context is not available at the starting phase, so it basically waits for the widget to initialise and then it assigns the variable. If you want I could put the assignment separately in initState for easier understanding.

Ah fair enough, I'm not used to this way of accessing buildcontext, never used it much tbh.

Okay, I'll change it to more commonly found syntax:

late EditorState editorState; @override void initState() { super.initState(); editorState = context.read<EditorState>(); }

Let me know if this looks good.

Actually, both of them look good to me. I prefer to the existing one, because no need to override the initState.

Xazin · 2023-06-12T17:46:34Z

...lutter/lib/plugins/document/presentation/editor_plugins/outline/outline_block_component.dart

+          child = BlockComponentActionWrapper(
+            node: widget.node,
+            actionBuilder: widget.actionBuilder!,
+            child: _buildOutlineBlock(),


Suggested change

child: _buildOutlineBlock(),

child: child,

It is not advisable to use 'child' here. The widget will not be updated within the builder if 'child' is returned; instead, we should rebuild the widget.

The below code is wrong too. It should return _buildOutlineBlock() directly, not the child.

Xazin · 2023-06-12T17:47:31Z

...lutter/lib/plugins/document/presentation/editor_plugins/outline/outline_block_component.dart

+          ),
+          Column(
+            crossAxisAlignment: CrossAxisAlignment.start,
+            children: headingNodes


Suggested change

children: headingNodes

children: getHeadingNodes()

No need to have a variable with this if you only use it in one place.

Xazin · 2023-06-12T17:48:53Z

...lutter/lib/plugins/document/presentation/editor_plugins/outline/outline_block_component.dart

+          const Divider(
+            color: Colors.white54,
+          ),
+          Column(


Do you need a Column here? You're already inside a Column with the exact same crossAxisAlignment.

Just do

...getHeadingNodes().map((e) => OutlineItemWidget(node: e)).toList(),

Yeah simply spreading the list of widgets would be a good idea. Thanks ✨

- Add localized strings - Fix minor suggested issues

AmanNegi · 2023-06-13T13:52:26Z

@LucasXu0 @Xazin I have made all the changes requested so far. If more improvements are needed please let me know.

Now I will be working on the tests for this plugin. If some resources/examples are available on how to write tests for editor-plugins that would be helpful as well. 🤝 😃

AmanNegi · 2023-06-24T04:40:10Z

@LucasXu0 @Xazin I have made all the changes requested so far. If more improvements are needed please let me know.

Now I will be working on the tests for this plugin. If some resources/examples are available on how to write tests for editor-plugins that would be helpful as well. 🤝 😃

cc @LucasXu0 Could you please guide me with this.

AmanNegi · 2023-06-28T05:59:24Z

@LucasXu0 I have added the outline tests. Could you please verify the tests.

LucasXu0 · 2023-06-28T06:00:08Z

@AmanNegi Sure.

Xazin · 2023-06-28T06:07:07Z

...lutter/lib/plugins/document/presentation/editor_plugins/outline/outline_block_component.dart

+extension on Node {
+  double get verticalSpacing {
+    if (type != HeadingBlockKeys.type) {
+      assert(false);


Just noticed this assert, feels like a hack to me. There are two similar ones further below.

because only node with heading type can fetch this values.

Yes, but assert(false) is not meaningful, and does not provide any information that would lead one to resolve it.

Hence why I feel that it is a hack.

It would make more sense to have assert(type == HeadingBlockKeys.type), as that would give a meaningful assertion error. I understand its use, and I'm not saying we should change it since it's only in debugging.

LucasXu0 · 2023-06-28T07:52:28Z

FYI, @AmanNegi. I refactor the UI part.

adding a hover style
using block selection instead of cursor
remove the content title\

AmanNegi · 2023-06-29T13:25:25Z

Merged! 🎉🎊 Thanks a lot for your support @LucasXu0.

feat: Add Outline Block

a14345f

LucasXu0 reviewed Jun 10, 2023

View reviewed changes

LucasXu0 added 2 commits June 11, 2023 22:08

feat: implement outline block component

cc7634f

Merge branch 'main' into create-outline

653bf8b

LucasXu0 reviewed Jun 11, 2023

View reviewed changes

Xazin reviewed Jun 12, 2023

View reviewed changes

AmanNegi and others added 3 commits June 13, 2023 19:08

Merge branch 'AppFlowy-IO:main' into create-outline

50a1df0

chore: Localize text strings

01ef7df

- Add localized strings - Fix minor suggested issues

chore: Merge branch 'main' into 'create-outline'

d5a556a

AmanNegi added 2 commits June 27, 2023 20:03

chore: merge into main

7826664

chore: Add tests

ee65078

fix: Remove irrelevant comments

54e6b38

Xazin reviewed Jun 28, 2023

View reviewed changes

feat: refactor UI issues and add tests

f0cc858

LucasXu0 changed the title ~~feat: Add Outline Block~~ feat: add outline block Jun 28, 2023

Merge remote-tracking branch 'origin/main' into create-outline

f4961fd

LucasXu0 added 2 commits June 29, 2023 10:51

chore: format code

0bbfb2c

chore: add missing import

be490e3

LucasXu0 merged commit 95d4fb6 into AppFlowy-IO:main Jun 29, 2023

	static const String type = 'outline_block';
	static const String type = 'outline';

	style: editorState.editorStyle.textStyleConfiguration.text,
	style: configuration.textStyle(node),

feat: add outline block #2750

feat: add outline block #2750

Conversation

AmanNegi commented Jun 9, 2023

Feature Preview

PR Checklist

AmanNegi commented Jun 9, 2023 • edited Loading

LucasXu0 commented Jun 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucasXu0 commented Jun 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmanNegi Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucasXu0 commented Jun 11, 2023

codecov bot commented Jun 11, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xazin Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmanNegi commented Jun 13, 2023

AmanNegi commented Jun 24, 2023 • edited Loading

AmanNegi commented Jun 28, 2023

LucasXu0 commented Jun 28, 2023

Xazin Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xazin Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucasXu0 commented Jun 28, 2023

AmanNegi commented Jun 29, 2023

AmanNegi commented Jun 9, 2023 •

edited

Loading

AmanNegi Jun 13, 2023 •

edited

Loading

codecov bot commented Jun 11, 2023 •

edited

Loading

Xazin Jun 12, 2023 •

edited

Loading

AmanNegi commented Jun 24, 2023 •

edited

Loading

Xazin Jun 28, 2023 •

edited

Loading

Xazin Jun 28, 2023 •

edited

Loading