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 title plugin #2078

Closed
wants to merge 66 commits into from
Closed

Feat title plugin #2078

wants to merge 66 commits into from

Conversation

rileyhawk1417
Copy link
Collaborator

@rileyhawk1417 rileyhawk1417 commented Mar 22, 2023

Description

The feature embeds a large text representing the Document Title.
Its a Editable Text Widget meaning the document name can be changed through the plugin.
This solves issue #1774

ToDo:

  • Display document name on page.
  • Save the new name to the document and update changes.

rileyhawk1417 and others added 30 commits July 8, 2022 16:54
@@ -189,7 +189,9 @@ class _AppFlowyEditorPageState extends State<_AppFlowyEditorPage> {
],
],
toolbarItems: [
smartEditItem,
if (openAIKey != null && openAIKey!.isNotEmpty) ...[
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could also make the custom getter for openAIKey return an empty string if null.

  String get openAIKey => documentBloc.state.userProfilePB?.openaiKey ?? '';

  ...

  if (openAIKey.isNotEmpty) ...[

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another option.

openAIKey?.isNotEmpty ?? false

@@ -240,7 +242,7 @@ class _AppFlowyEditorPageState extends State<_AppFlowyEditorPage> {
}
}

dartz.Tuple2<bool, Selection?> _autoFocusParameters() {
dartz.Tuple2<bool, Selection?> _autoFocusParamters() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why rename, it was correctly spelled before?

Suggested change
dartz.Tuple2<bool, Selection?> _autoFocusParamters() {
dartz.Tuple2<bool, Selection?> _autoFocusParameters() {

@Xazin
Copy link
Collaborator

Xazin commented Mar 26, 2023

Starting to look really good, can't wait to test this out when you're done 👍

@annieappflowy
Copy link
Collaborator

You @Xazin are awesome too! Thanks for providing valuable feedback and offering help!

Node(type: kCoverType),
);
transaction
.insertNodes([0], [Node(type: kCoverType), Node(type: kTitleType)]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

transaction.insertNodes(
        [0],
        [
          Node(type: kCoverType),
          Node(type: kTitleType),
        ],
      );

Add a comma here.

import 'package:appflowy_editor/appflowy_editor.dart';

const String kTitleType = 'title';
const String kTitleAttribute = 'docBloc';
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 unused code.

const String kTitleAttribute = 'docBloc';

key: context.node.key,
node: context.node,
editorState: context.editorState,
title: docBloc);
Copy link
Collaborator

Choose a reason for hiding this comment

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

pls add a comma between docBloc and ).

@rileyhawk1417 rileyhawk1417 marked this pull request as ready for review April 4, 2023 13:16
@@ -130,11 +129,11 @@ class _AppFlowyEditorPageState extends State<_AppFlowyEditorPage> {
@override
Widget build(BuildContext context) {
final theme = Theme.of(context);
final autoFocusParameters = _autoFocusParameters();
final autoFocusParamters = _autoFocusParamters();
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a typo in "parameters" here

const String kTitleType = 'title';

class TitleNodeWidgetBuilder extends NodeWidgetBuilder {
TitleNodeWidgetBuilder({required this.docBloc});
Copy link
Contributor

Choose a reason for hiding this comment

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

If all fields are final, can't this constructor be const?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I get a warning about removing the const keyword. Would it be better to just not make the field not final?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, it's because NodeWidgetBuilder does not have a const constructor.

@override
void initState() {
super.initState();
docBloc = context.read<DocumentBloc>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since docBloc is marked as late, you can initialize it during its instantiating instead of in initState.

docBloc = context.read<DocumentBloc>();
textEditingController = TextEditingController.fromValue(
TextEditingValue(
text: docBloc!.view.name.toString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

If you use the null assertion operator here, shouldn't you make docBloc required? It's still possible for this class to be initialized with a docBloc with a value of null, and this null assertion will cause the program to fail.

focusNode: focusNode,
selectionColor: theme.highlightColor,
style: TextStyle(
color: textColor, fontSize: 50.0, fontWeight: FontWeight.bold),
Copy link
Contributor

Choose a reason for hiding this comment

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

50, as a hardcoded size. Can we get that size from the theme file instead? See the AppFlowy style guide...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay will put it in a theme file, had tried some of the defaults such as Theme.of(context).textTheme.titleMedium and a few others. They didn't give the best result.

autoFocus: autoFocusParameters.value1,
focusedSelection: autoFocusParameters.value2,
autoFocus: autoFocusParamters.value1,
focusedSelection: autoFocusParamters.value2,
Copy link
Collaborator

@Xazin Xazin Apr 4, 2023

Choose a reason for hiding this comment

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

And here


final Node node;
final EditorState editorState;
final DocumentBloc? title;
Copy link
Collaborator

@Xazin Xazin Apr 4, 2023

Choose a reason for hiding this comment

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

Do you need this? It's unused right?

You pass it through the Builder, but you in initState you read it from context anyway.

Also the name title is a bit ambiguous for a DocumentBloc


class _TitleNodeWidgetState extends State<_TitleNodeWidget> {
_TitleNodeWidgetState();
late DocumentBloc? docBloc;
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
late DocumentBloc? docBloc;
late final DocumentBloc docBloc;

Comment on lines 86 to 88
onSubmitted: (value) {
updateName(value);
},
Copy link
Collaborator

@Xazin Xazin Apr 4, 2023

Choose a reason for hiding this comment

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

Suggested change
onSubmitted: (value) {
updateName(value);
},
onSubmitted: updateName,

Comment on lines 68 to 70
void updateName(String value) async {
await service.updateView(viewId: docBloc!.view.id, name: value);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be private

Comment on lines 22 to 25
@override
NodeValidator<Node> get nodeValidator => (node) {
return true;
};
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
@override
NodeValidator<Node> get nodeValidator => (node) {
return true;
};
@override
NodeValidator<Node> get nodeValidator => (node) => true;

@appflowy
Copy link
Contributor

@LucasXu0 Is this PR ready to merge?

@LucasXu0
Copy link
Collaborator

@LucasXu0 Is this PR ready to merge?

I don't think so. There are still some things that need to be done, such as adding support for multiple selection in the title plugin, and allowing the user to navigate to the next line using the enter key or arrow down key.

By the way, @rileyhawk1417, how about submitting this PR to the appflowy_editor repository instead? The main repository won't run the editor test on CI anymore.

@LucasXu0 LucasXu0 marked this pull request as draft April 12, 2023 03:48
@rileyhawk1417
Copy link
Collaborator Author

Okay @LucasXu0 will move it to appflowy_editor, then close this PR. Will open a new one when connecting the plugin to AppFlowy.

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.

None yet

6 participants