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

Allow developer to change toolbar color as well as option to show default toolbar items and html to document converter added #58

Merged
merged 12 commits into from
Apr 14, 2023

Conversation

alihassan143
Copy link
Contributor

this pr added option to add custom color to toolbar and elevation also exported extension so it can be accessed when adding custom toolbar items

@CLAassistant
Copy link

CLAassistant commented Apr 11, 2023

CLA assistant check
All committers have signed the CLA.

@alihassan143 alihassan143 changed the title Allow developer to change toolbar color as well as option to show default toolbar items Allow developer to change toolbar color as well as option to show default toolbar items and html to document converter added Apr 11, 2023
@@ -34,6 +34,9 @@ class AppFlowyEditor extends StatefulWidget {
this.autoFocus = false,
this.focusedSelection,
this.customActionMenuBuilder,
this.toolbarColor=const Color(0xFF333333),
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not a good idea to add these parameters to editor_service. Instead, you can refer to editor_style.dart for guidance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but inside toolbar widget color is already defined if we did add color inside theme than its always be overridden by the already added color inside Material widget

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand what you mean. What I meant was to add the toolbar here.

Screenshot 2023-04-12 at 21 00 27

@LucasXu0
Copy link
Collaborator

Hey @alihassan143, please format the code before submitting, dart format .

@alihassan143
Copy link
Contributor Author

@LucasXu0 code is formated

@alihassan143
Copy link
Contributor Author

alihassan143 commented Apr 12, 2023

@LucasXu0 CompositedTransformFollower instead of this widget if we use textselectiontoolbar widget and pass childrens to its if the child list grows its adapts automatically to screen width

@alihassan143
Copy link
Contributor Author

@LucasXu0 toolbar elevation and toolbar color now moved to editor style

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you made a barrel file for src/extenstions/... then you can remove all duplicate exports.

eg. export 'src/extensions/node_extensions.dart'; and export 'src/extensions/attributes_extension.dart';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are still some pieces of code that need to be formatted. You can run dart fix --apply locally to apply the changes.

@LucasXu0
Copy link
Collaborator

This PR reminds me that I should split the toolbar function into a plugable feature. The editor and the toolbar should not have a reference to each other, but they can be composed together for use.

@Xazin
Copy link
Collaborator

Xazin commented Apr 13, 2023

By the way, workflows have not run, but you need to reword all your commits to conform to the commit format.

It's a good practice to learn, I was happily surprised when I found out AppFlowy already enforces commit messages the way I prefer them.

Honestly I think for you it would be easier if you squashed all your commits into one, and then made the commit message eg. feat: abstract toolbar colors to styling.

If you want to do the squash, you can do it like this:

git reset --soft HEAD~12
git commit -m "feat: abstract toolbar colors to editor style"

If you want to reword each of your commits to conform to the commit lint, you can do:

git rebase HEAD~12

Then an interactive editor will open, press a to start inserting, delete all the pick at the beginning and put an r instead. r is short for reword.

After. that hit ESC and then write :wq!, then 12 times an editor will open, and you can press a to edit the commit message, then when done just do ESC and :wq!.

Thus preferring the squash..

EDIT: And of course, when you're done, you need to force push because you changed the history. git push --force

@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Merging #58 (1bbe367) into main (cf5e83d) will increase coverage by 0.01%.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##             main      #58      +/-   ##
==========================================
+ Coverage   86.65%   86.67%   +0.01%     
==========================================
  Files         124      124              
  Lines        7230     7248      +18     
==========================================
+ Hits         6265     6282      +17     
- Misses        965      966       +1     
Impacted Files Coverage Δ
lib/src/render/action_menu/action_menu_item.dart 100.00% <ø> (ø)
lib/src/render/rich_text/checkbox_text.dart 98.00% <ø> (ø)
lib/src/render/style/plugin_styles.dart 88.33% <ø> (ø)
lib/src/render/toolbar/toolbar_item.dart 93.77% <ø> (ø)
...efault_text_operations/format_rich_text_style.dart 86.66% <ø> (ø)
...nternal_key_event_handlers/copy_paste_handler.dart 18.90% <ø> (ø)
...event_handlers/markdown_syntax_to_styled_text.dart 75.12% <ø> (ø)
lib/src/service/toolbar_service.dart 93.33% <66.66%> (-2.02%) ⬇️
lib/src/infra/html_converter.dart 88.49% <100.00%> (+0.31%) ⬆️
lib/src/render/style/editor_style.dart 100.00% <100.00%> (ø)
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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