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: New way to get Delta from HTML inputs #1984

Merged
merged 2 commits into from
Jul 7, 2024
Merged

Feat: New way to get Delta from HTML inputs #1984

merged 2 commits into from
Jul 7, 2024

Conversation

CatHood0
Copy link
Collaborator

@CatHood0 CatHood0 commented Jul 7, 2024

Description

The old implementation was changed to obtain a Delta from an HTML input, where attributes that were useful for other people were lost (almost always) because Markdown did not support them (such as subscript, superscript, color, background, video, underline).

Now with this implementation (using flutter_quill_delta_from_html), we can not only get a Delta with all the attributes currently supported, but also added support for rendering custom HTML for that becomes a valid Quill js Operation

Related Issues

Related #1982
Fix #1947
Fix #1945
Fix #1922

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I titled the PR using Conventional Commits.
  • I did not modify the CHANGELOG.md nor the package version in pubspec.yaml files.
  • All existing and new tests are passing.
  • I have run the commands in ./scripts/before_push.sh and it all passed successfully

Breaking Change

Does your PR require developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate that with a ! in the title as explained in Conventional Commits).
  • No, this is not a breaking change.

@CatHood0 CatHood0 requested a review from EchoEllet July 7, 2024 20:43
@singerdmx singerdmx merged commit e12d64e into singerdmx:master Jul 7, 2024
2 checks passed
@EchoEllet
Copy link
Collaborator

EchoEllet commented Jul 7, 2024

If the markdown converter package is no longer needed, I suggest extracting the changes made on this repository and sending it to the upstream repository.

We still need it for Markdown pasting.

The html2md package is no longer required as a dependency, though.

https://github.com/singerdmx/flutter-quill/blob/master/pubspec.yaml#L59

@singerdmx
Copy link
Owner

Do you mean this #1985 ?

@EchoEllet
Copy link
Collaborator

Do you mean this #1985 ?

Yes.

@singerdmx
Copy link
Owner

Why are those dependencies in the core?
Shouldn't they be in https://github.com/singerdmx/flutter-quill/blob/master/quill_html_converter/pubspec.yaml ?

@EchoEllet
Copy link
Collaborator

flutter_quill_delta_from_html

As a suggestion, We can extract the required parts that are not specific to Flutter from flutter_quill so your package can use them to extend the support on Dart frameworks outside of Flutter?

This will allow using the converter on the server side.

@EchoEllet
Copy link
Collaborator

Why are those dependencies in the core?
Shouldn't they be in https://github.com/singerdmx/flutter-quill/blob/master/quill_html_converter/pubspec.yaml ?

Previously, we needed them for the rich text pasting feature. Some of them were required in the HTML converter, though I duplicated them since we only need to convert from HTML to avoid adding extra dependencies on flutter_quill by requiring quill_html_converter.

@CatHood0
Copy link
Collaborator Author

CatHood0 commented Jul 7, 2024

We can extract the required parts that are not specific to Flutter from flutter_quill so your package can use them to extend the support on Dart frameworks outside of Flutter?

No problem! My implementation is made to be used by everyone, however, I did not take this case into account. I'll take your suggestion for the next update from my package

@EchoEllet
Copy link
Collaborator

EchoEllet commented Jul 7, 2024

No problem! My implementation is made to be used by everyone, however, I did not take this case into account. I'll take your suggestion for the next update from my package

Though this is not a technical issue, The package is called flutter_quill_delta_from_html which might confuse users in case they use the package for Dart projects that are not Flutter, you don't have to solve this since it's not an issue though there are a few suggestions:

  1. Discounting the package and publishing it again with a different name since usually, the developer will use flutter_quill which already has this functionality that depends on flutter_quill_delta_from_html .

  2. Move the package to this repo directly for seamless integration with the project, so we can automate the releases with CI, and tests, making it a bit easier to contribute to the package code and make some changes in the same PR without sending two different PRs to different repositories, bug report, feature request etc...

If we introduce breaking changes (10.0.0), we can update the quill_delta_from_html package so it has the same version as the other packages.

Can be more future-proof, Kotlin 2.0.0 did something similar by moving the Compose Compiler in the Kotlin repository instead of being two separate projects. Eliminating the need for having a table for the compatible versions and having to also manage them.

Or maybe we can move the code of this package directly to a source directory in flutter_quill since it depends on it though this will make it a bit harder for projects that use it for the backend or non-Flutter projects that use Dart.

In case you're interested, I would recommend creating a discussion to plan things before releasing them since we will have two packages, for converting to and from HTML.

@EchoEllet
Copy link
Collaborator

EchoEllet commented Jul 7, 2024

When testing this change, I had a build failure:
Invalid depfile: /Users/ellet/Developer/playground/framework_based/flutter/flutter-quill/example/.dart_tool/flutter_build/d81a32a1e04fb31b0404583761919a5b/kernel_snapshot.d
Error: Couldn't resolve the package 'flutter_quill_delta_from_html' in 'package:flutter_quill_delta_from_html/flutter_quill_delta_from_html.dart'.
../lib/src/models/documents/delta_x.dart:1:8: Error: Not found: 'package:flutter_quill_delta_from_html/flutter_quill_delta_from_html.dart'
import 'package:flutter_quill_delta_from_html/flutter_quill_delta_from_html.dart';
       ^
../lib/src/models/documents/delta_x.dart:25:48: Error: Type 'CustomHtmlPart' not found.
  static Delta fromHtml(String htmlText, {List<CustomHtmlPart>? customBlocks}) {
                                               ^^^^^^^^^^^^^^
../lib/src/models/documents/delta_x.dart:25:48: Error: 'CustomHtmlPart' isn't a type.
  static Delta fromHtml(String htmlText, {List<CustomHtmlPart>? customBlocks}) {
                                               ^^^^^^^^^^^^^^
../lib/src/models/documents/delta_x.dart:26:25: Error: Method not found: 'HtmlToDelta'.
    final htmlToDelta = HtmlToDelta(customBlocks: customBlocks);
                        ^^^^^^^^^^^
Unhandled exception:
FileSystemException(uri=org-dartlang-untranslatable-uri:package%3Aflutter_quill_delta_from_html%2Fflutter_quill_delta_from_html.dart; message=StandardFileSystem only supports file:* and data:* URIs)
#0      StandardFileSystem.entityForUri (package:front_end/src/api_prototype/standard_file_system.dart:34)
#1      asFileUri (package:vm/kernel_front_end.dart:752)
#2      writeDepfile (package:vm/kernel_front_end.dart:892)
<asynchronous suspension>
#3      FrontendCompiler.compile (package:frontend_server/frontend_server.dart:680)
<asynchronous suspension>
#4      starter (package:frontend_server/starter.dart:101)
<asynchronous suspension>
#5      main (file:///Volumes/Work/s/w/ir/x/w/sdk/pkg/frontend_server/bin/frontend_server_starter.dart:13)
frontend_server_starter.dart:13
<asynchronous suspension>

Target kernel_snapshot failed: Exception

Command PhaseScriptExecution failed with a nonzero exit code
** BUILD FAILED **

That can be fixed by:

(cd example && flutter clean)
flutter pub get

This issue will only occur while development with the example, if we make changes to the packages, we will have to manually execute flutter pub get in the example directory.

It seems that the new converter code experiencing some issues:

image

Run the Example application on any platform, navigate to the Default screen, and click on this dropdown button in the Scaffold Toolbar actions:

image

This will convert the Delta to HTML, then convert it back with DeltaX.fromHtml

@CatHood0 CatHood0 deleted the support_for_better_html_to_delta branch July 11, 2024 01:44
@CatHood0
Copy link
Collaborator Author

CatHood0 commented Jul 12, 2024

The package is called flutter_quill_delta_from_html which might confuse users in case they use the package for Dart projects that are not Flutter

I don't know why I wasn't notified of this comment sooner.

Now that you mention it, I also thought about that at the beginning, only since I had an error with another package with a similar name, to avoid breaking my brain, I better just name it whatever seemed convenient. However, now I decided that it was better to name it in a simple way without including flutter_quill since the package is responsible for using a valid Delta format for Quill Js and flutter_quill . I'm still deciding whether to remove the line-height parsing because Quill Js originally doesn't support this feature by default. Now it's called quill_delta_from_html

@EchoEllet
Copy link
Collaborator

EchoEllet commented Jul 13, 2024

I don't know why I wasn't notified of this comment sooner.

Seems to be a notifications issue.

I also thought about that at the beginning, only since I had an error with another package with a similar name, to avoid breaking my brain

Naming can be challenging

Sometimes it takes more time to decide the name than it takes to write the actual functionality in addition to the tests.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants