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: add outline block #2750

Merged
merged 13 commits into from
Jun 29, 2023
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
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/database/referenced_database_menu_tem.dart';
import 'package:appflowy/plugins/document/presentation/editor_plugins/plugins.dart';
import 'package:appflowy/plugins/document/presentation/editor_style.dart';
import 'package:appflowy_editor/appflowy_editor.dart';
Expand Down Expand Up @@ -56,6 +55,7 @@ class _AppFlowyEditorPageState extends State<AppFlowyEditorPage> {
codeBlockItem,
emojiMenuItem,
autoGeneratorMenuItem,
outlineItem,
];

late final Map<String, BlockComponentBuilder> blockComponentBuilders =
Expand Down Expand Up @@ -209,6 +209,7 @@ class _AppFlowyEditorPageState extends State<AppFlowyEditorPage> {
AutoCompletionBlockKeys.type: AutoCompletionBlockComponentBuilder(),
SmartEditBlockKeys.type: SmartEditBlockComponentBuilder(),
ToggleListBlockKeys.type: ToggleListBlockComponentBuilder(),
OutlineBlockKeys.type: OutlineBlockComponentBuilder(),
};

final builders = {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,232 @@
import 'dart:async';

import 'package:appflowy_editor/appflowy_editor.dart';
import 'package:flutter/material.dart';
import 'package:provider/provider.dart';

class OutlineBlockKeys {
const OutlineBlockKeys._();

static const String type = 'outline_block';
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Screenshot 2023-06-13 at 17 57 50

iconData: Icons.clear_all,
Copy link
Collaborator

Choose a reason for hiding this comment

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

clear_all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

icons_selection

Copy link
Collaborator

Choose a reason for hiding this comment

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

The list_alt looks better.

keywords: ['outline', 'table of contents'],
nodeBuilder: (editorState) => outlineBlockNode(),
replace: (_, node) => node.delta?.isEmpty ?? false,
);

Node outlineBlockNode() {
return Node(
type: OutlineBlockKeys.type,
);
}

class OutlineBlockComponentBuilder extends BlockComponentBuilder {
OutlineBlockComponentBuilder({
this.configuration = const BlockComponentConfiguration(),
});

@override
final BlockComponentConfiguration configuration;

@override
BlockComponentWidget build(BlockComponentContext blockComponentContext) {
final node = blockComponentContext.node;
return OutlineBlockWidget(
key: node.key,
node: node,
configuration: configuration,
showActions: showActions(node),
actionBuilder: (context, state) => actionBuilder(
blockComponentContext,
state,
),
);
}

@override
bool validate(Node node) => true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

validate the outline_block.children must be empty.

}

class OutlineBlockWidget extends BlockComponentStatefulWidget {
const OutlineBlockWidget({
super.key,
required super.node,
super.showActions,
super.actionBuilder,
super.configuration = const BlockComponentConfiguration(),
});

@override
State<OutlineBlockWidget> createState() => _OutlineBlockWidgetState();
}

class _OutlineBlockWidgetState extends State<OutlineBlockWidget>
with BlockComponentConfigurable {
@override
BlockComponentConfiguration get configuration => widget.configuration;

@override
Node get node => widget.node;

late EditorState editorState = context.read<EditorState>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Late but you assign it a value immediately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

late Stream<Transaction> stream;

@override
void initState() {
super.initState();

stream = editorState.transactionStream;
}

@override
Widget build(BuildContext context) {
Widget child = _buildOutlineBlock();

return StreamBuilder(
stream: stream,
builder: (context, snapshot) {
if (widget.showActions && widget.actionBuilder != null) {
child = BlockComponentActionWrapper(
node: widget.node,
actionBuilder: widget.actionBuilder!,
child: _buildOutlineBlock(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
child: _buildOutlineBlock(),
child: child,

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

);
}
return child;
},
);
}

Widget _buildOutlineBlock() {
final headingNodes = getHeadingNodes();
return Container(
padding: const EdgeInsets.symmetric(
horizontal: 15.0,
vertical: 20.0,
),
decoration: BoxDecoration(
border: Border.all(color: Colors.white54),
borderRadius: BorderRadius.circular(15.0),
),
child: Column(
crossAxisAlignment: CrossAxisAlignment.start,
children: [
Text(
"TABLE OF CONTENTS: ",
Copy link
Collaborator

Choose a reason for hiding this comment

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

l10n

Copy link
Contributor Author

@AmanNegi AmanNegi Jun 13, 2023

Choose a reason for hiding this comment

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

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.

style: editorState.editorStyle.textStyleConfiguration.text,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

),
const Divider(
color: Colors.white54,
),
Column(
Copy link
Contributor

@Xazin Xazin Jun 12, 2023

Choose a reason for hiding this comment

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

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(),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

crossAxisAlignment: CrossAxisAlignment.start,
children: headingNodes
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
children: headingNodes
children: getHeadingNodes()

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

.map(
(e) => OutlineItemWidget(node: e),
)
.toList(),
)
],
),
);
}

Iterable<Node> getHeadingNodes() {
final children = editorState.document.root.children;
return children.where((element) => element.type == HeadingBlockKeys.type);
}
}

class OutlineItemWidget extends StatelessWidget {
OutlineItemWidget({
super.key,
required this.node,
}) {
assert(node.type == HeadingBlockKeys.type);
}

final Node node;

@override
Widget build(BuildContext context) {
final editorState = context.read<EditorState>();
return MouseRegion(
cursor: SystemMouseCursors.click,
child: GestureDetector(
onTap: () {
// when clicked scroll the view to the heading

editorState.updateSelectionWithReason(
Selection.single(
path: node.path,
startOffset: node.delta?.length ?? 0,
),
reason: SelectionUpdateReason.uiEvent,
);
},
child: Container(
margin: EdgeInsets.only(top: node.verticalSpacing),
padding: EdgeInsets.only(left: node.leftIndent),
child: Text(
node.outlineItemText,
style: editorState.editorStyle.textStyleConfiguration.text,
),
),
),
);
}
}

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

@Xazin Xazin Jun 28, 2023

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

because only node with heading type can fetch this values.

Copy link
Contributor

@Xazin Xazin Jun 28, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed.

return 0.0;
}
final level = attributes[HeadingBlockKeys.level];
if (level == 1) {
return 10;
} else if (level == 2) {
return 8;
}
return 5;
}

double get leftIndent {
if (type != HeadingBlockKeys.type) {
assert(false);
return 0.0;
}
final level = attributes[HeadingBlockKeys.level];
if (level == 2) {
return 15;
} else if (level == 3) {
return 60;
}
return 0;
}

String get outlineItemText {
if (type != HeadingBlockKeys.type) {
assert(false);
return '';
}
final delta = this.delta;
if (delta == null) {
return '';
}
final text = delta.toPlainText();
final level = attributes[HeadingBlockKeys.level];
if (level == 2) {
return '∘ $text';
} else if (level == 3) {
return '- $text';
}
return text;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ export 'cover/cover_image_picker.dart';
export 'emoji_picker/emoji_menu_item.dart';
export 'extensions/flowy_tint_extension.dart';
export 'database/inline_database_menu_item.dart';
export 'database/referenced_database_menu_item.dart';
export 'database/database_view_block_component.dart';
export 'math_equation/math_equation_block_component.dart';
export 'openai/widgets/auto_completion_node_widget.dart';
export 'openai/widgets/smart_edit_node_widget.dart';
export 'openai/widgets/smart_edit_toolbar_item.dart';
export 'toggle/toggle_block_component.dart';
export 'toggle/toggle_block_shortcut_event.dart';
export 'outline/outline_block_component.dart';