[PR] Adding `flutter-quill` based editor

LuchoTurtle commented 9 months ago

This is still in progress. It will add the editor with the work that was made in https://github.com/dwyl/flutter-wysiwyg-editor-tutorial and embed it in the app.

It features some changes to the pre-existing bloc, now using the sealed classes feature (see https://medium.com/@aliammariraq/sealed-classes-in-flutter-unlocking-powerful-features-b73ab5838ca0).

It also adds an AppCubit, which is meant to hold all general-purpose app states.

LuchoTurtle commented 9 months ago

Many of the constraints that I had with flutter-quill seem to be addressed (already merged - see https://github.com/singerdmx/flutter-quill/commits/master). One of them pertained to gallery-saver, which constrained dependency versioning (this was included in the work that I made at https://github.com/visual-space/visual-editor) and is already merged in https://github.com/singerdmx/flutter-quill/tree/master/flutter_quill_extensions.

However, I'm still waiting for the package to be upgraded to use it (see my comment in https://github.com/singerdmx/flutter-quill/pull/1403#issuecomment-1737079301).

nelsonic commented 9 months ago

@LuchoTurtle great that flutter-quill has been updated. Thanks for keeping an eye on this and reporting back. 👌

LuchoTurtle commented 9 months ago

I've made significant progress. The app is functional, as it stands. I'm not yet submitting for review because:

the test coverage is not at 100%, due to https://github.com/singerdmx/flutter-quill/issues/1389 and https://github.com/Fintasys/emoji_picker_flutter/issues/156.
items should only be shown in the title of the rich-text editor document. I'm saving the document file with the item, I just need to splice the content so it doesn't show all the text in lengthy items.

The first point is, I'm afraid, a hard thing to get going because, as explained in the issues, there doesn't seem to have a discernable way of simulating image picking and emoji picking on unit tests, which doesn't yield any results during coverage.

codecov[bot] commented 9 months ago

Codecov Report

Merging #341 (75e373d) into main (c72ffe3) will decrease coverage by 6.49%. The diff coverage is 90.13%.

@@             Coverage Diff             @@
##              main     #341      +/-   ##
===========================================
- Coverage   100.00%   93.51%   -6.49%     
===========================================
  Files           11       24      +13     
  Lines          281      447     +166     
===========================================
+ Hits           281      418     +137     
- Misses           0       29      +29

Files	Coverage Δ
lib/blocs/app/app_cubit.dart	`100.00% <100.00%> (ø)`
lib/blocs/app/app_state.dart	`100.00% <100.00%> (ø)`
lib/blocs/data/data_cubit.dart	`100.00% <100.00%> (ø)`
lib/blocs/data/data_state.dart	`100.00% <100.00%> (ø)`
lib/blocs/item/item_bloc.dart	`100.00% <100.00%> (ø)`
lib/blocs/item/item_event.dart	`100.00% <100.00%> (ø)`
lib/blocs/item/item_state.dart	`100.00% <100.00%> (ø)`
lib/core/app.dart	`100.00% <100.00%> (ø)`
lib/core/data_layer.dart	`100.00% <100.00%> (ø)`
lib/data/repositories/image/image_repository.dart	`100.00% <100.00%> (ø)`
... and 12 more

nelsonic commented 9 months ago

Thanks for the update. It's looking good so far. 👌

nelsonic commented 9 months ago

For your own benefit as much as mine, when are giving a Demo, try to input valid-looking data:

It doesn't take much longer to write "Buy bananas" or "Make Lunch" than it does to write "asadadasdas". If you're showcasing a feature to someone always show the attention to detail to simulate a "real" person using it. It instantly makes you look more professional about your work. 🕴️ Whereas inputting garbage data screams "don't care about this". 🙄

Otherwise, looking good. 👌

LuchoTurtle commented 9 months ago

I've added several more tests so I could increase the coverage as much as I could (given the constraints detailed above). Even though I managed to do so, I still ran into some problems with integration testing with image picking.

I've made some changes to how the web and mobile image callbacks are made so they're easier to unit test (since I can't get them to work on integration tests). However, I've had progress with integration testing emojis, so that's a ➕.

I've also had success with overriding PathProviderPlatform so tests would actually pass in a contained fashion. This means that this may be a possible avenue to actually get image picking to work on integration tests and get the coverage as high as I can. I'll give this a whirl tomorrow.

In addition to this, I've found a bug with navigation and I'm wanting to print only the title when adding items. So, I want to get all of these issues sorted first before submitting for review.

LuchoTurtle commented 9 months ago

I'm submitting this for review:

figured out a way of overriding the ImagePicker. The coverage is not at 100% because it is not possible to override the kIsWeb variable (that is used in flutter-quill) during tests. However, we can get around this by using a CustomButton that has the same behaviour as ImageButton (where the offending line is, see https://github.com/singerdmx/flutter-quill/blob/8cb14b8155219a94c612467df79206ff09b9b386/flutter_quill_extensions/lib/embeds/toolbar/media_button.dart#L120), so we can override it in our tests with a fake injected dependency.
fixed the navigation bug where if the stack was empty, the person would go to a black screen.
item cards are only shown the very first sentence/title.

Test coverage should significantly improve. I want to implement these changes in https://github.com/dwyl/flutter-wysiwyg-editor-tutorial with the emoji and imagePicker test overrides so we could bump those coverage numbers up, as well.

LuchoTurtle commented 9 months ago

This is failing due to coverage being 93%. I don't know if you wish to change this setting temporarily so it passes, but I wager this PR is big enough to get merged.

nelsonic commented 9 months ago

Can't see where to (temporarily) lower the coverage checking threshold on CodeCov ... https://app.codecov.io/gh/dwyl/app

Editor works though:

Could not upload images in Chrome.

Tried it in the iOS Simulator ... https://stackoverflow.com/a/77234023/1148249 Couldn't get flutter run to boot the Simulator ... see: https://github.com/dwyl/learn-flutter/issues/97 Is there a "One Weird Trick"?

LuchoTurtle commented 9 months ago

I find this behaviour weird, since I'm using the same method on https://github.com/dwyl/flutter-wysiwyg-editor-tutorial/issues/5#issuecomment-1723331909.

I'll look into it. I'll take the opportunity and actually use BLoCs to raise events and change the UI accordingly (if there's a failure on posting images, like there's now) -> https://github.com/felangel/bloc/blob/master/docs/architecture.md

LuchoTurtle commented 9 months ago

The fix for the image upload is fixed, so it should work on all platforms. Please merge https://github.com/dwyl/imgup/pull/132 first, as it needs to be merged first for the tests to work (one is failing because of this unmerged change), and fix the tests that are currently failing in this PR. After merging, it should work (it does with the changes made to the server in localhost).

I've implemented a BLoC architecture, so it's congruent with other Flutter projects. I've followed a rough guide, as found in https://github.com/felangel/bloc/blob/master/docs/architecture.md.

The test coverage should also increase. It's not still at 100% because, as stated before, I can't force the widget tests to have the kIsWeb variable to true, as it's used inside flutter-quill.

nelsonic commented 9 months ago

@LuchoTurtle https://github.com/dwyl/imgup/pull/132 merged. Please confirm uploading is working from the app (this PR) thanks. 🙏

LuchoTurtle commented 9 months ago

If these tests pass, it should work. One of the integration tests does a call to the API. ⏳

LuchoTurtle commented 9 months ago

The tests seem to pass, meaning the API works. The reason the pipeline is failing is because of the code coverage reducing because of web embeds, as described previously (and in https://github.com/dwyl/flutter-wysiwyg-editor-tutorial/issues/12).

nelsonic commented 9 months ago

Ok, Let's ship this then. Please implement your strategy for custom button to mitigate the web embeds. 👌

nelsonic commented 9 months ago

I'm still unable to run this on the Simulator on my localhost:

Launching lib/main.dart on iPhone SE (3rd generation) in debug mode...
Running Xcode build...
Xcode build done.                                           12.1s
Failed to build iOS app
Error (Xcode): type argument 'nw_proxy_config_t' (aka 'struct nw_proxy_config *') is neither an Objective-C object nor a block type
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator17.0.sdk/System/Library/Frameworks/WebKit.framework/Headers/WKWebsiteDataStore.h:119:46

Parse Issue (Xcode): Could not build module 'WebKit'
/app/build/ios/Debug-iphonesimulator/flutter_inappwebview/flutter_inappwebview.framework/Headers/flutter_inappwebview-Swift.h:893:8

Could not build the application for the simulator.
Error launching application on iPhone SE (3rd generation).

So not able to fully test this PR. Related to: https://github.com/dwyl/learn-flutter/issues/97

dwyl / app

[PR] Adding `flutter-quill` based editor #341

Codecov Report