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

Title Node Plugin For AppFlowy Editor #392

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

rileyhawk1417
Copy link
Contributor

This plugin has been migrated from the AppFlowy repo

The original issue can be found here

@Xazin Xazin self-requested a review August 17, 2023 07:49
@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (af8d96b) 80.41% compared to head (bfd2bfe) 80.84%.

❗ Current head bfd2bfe differs from pull request most recent head 065c8aa. Consider uploading reports for the commit 065c8aa to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #392      +/-   ##
==========================================
+ Coverage   80.41%   80.84%   +0.42%     
==========================================
  Files         281      253      -28     
  Lines       11863    10331    -1532     
==========================================
- Hits         9540     8352    -1188     
+ Misses       2323     1979     -344     
Files Coverage Δ
...nent/service/renderer/block_component_service.dart 67.50% <100.00%> (-2.96%) ⬇️
lib/src/editor_state.dart 84.45% <100.00%> (-0.16%) ⬇️
...t/title_block_component/title_block_component.dart 90.19% <90.19%> (ø)

... and 136 files with indirect coverage changes

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

@Xazin Xazin requested a review from LucasXu0 August 17, 2023 12:41
@Xazin
Copy link
Collaborator

Xazin commented Sep 4, 2023

Hey @LucasXu0 can you help take a look at this?

I'm not a fan of the implementation itself, maybe you have a better method in mind?

@Xazin
Copy link
Collaborator

Xazin commented Sep 14, 2023

@LucasXu0

I see two issues still here, what is your take on them:

  1. I'm not a fan of the way we're currently enforcing the title block to always be the first node in a document, do you have some ideas for another route I could look at for improving this?
  2. Currently it's possible to select and add attributes to a title, I assume we should disallow this and only allow for plain text / UTF emojis. Since formatting as a list or quote, breaks the plugin.

Once we can merge this, I can continue work on AppFlowy to remove the title bar and abstract the share button and font settings out of the title bar as well.

@Xazin
Copy link
Collaborator

Xazin commented Sep 19, 2023

@LucasXu0

I see two issues still here, what is your take on them:

  1. I'm not a fan of the way we're currently enforcing the title block to always be the first node in a document, do you have some ideas for another route I could look at for improving this?
  2. Currently it's possible to select and add attributes to a title, I assume we should disallow this and only allow for plain text / UTF emojis. Since formatting as a list or quote, breaks the plugin.

Once we can merge this, I can continue work on AppFlowy to remove the title bar and abstract the share button and font settings out of the title bar as well.

Will address the 2nd issue and then merge, to continue with the improvements on AppFlowy.

@Xazin Xazin self-assigned this Sep 19, 2023
- The toolbar would format the title node, which is not meant to happen.
- Disabling the toolbar for the node ensures that it can't be turned into something else i.e list item.
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

2 participants