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: html encode parser added #314

Merged
merged 14 commits into from
Jul 20, 2023
Merged

Conversation

alihassan143
Copy link
Contributor

@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #314 (c99a570) into main (33b18d9) will increase coverage by 2.61%.
The diff coverage is 97.51%.

@@            Coverage Diff             @@
##             main     #314      +/-   ##
==========================================
+ Coverage   73.02%   75.64%   +2.61%     
==========================================
  Files         236      247      +11     
  Lines        9477     9703     +226     
==========================================
+ Hits         6921     7340     +419     
+ Misses       2556     2363     -193     
Impacted Files Coverage Δ
lib/src/plugins/html/html_document_decoder.dart 78.12% <83.33%> (-4.79%) ⬇️
...plugins/html/encoder/parser/image_node_parser.dart 88.23% <88.23%> (ø)
...b/src/plugins/html/encoder/delta_html_encoder.dart 96.36% <96.36%> (ø)
lib/src/infra/html_converter.dart 79.62% <100.00%> (ø)
...html/encoder/parser/bulleted_list_node_parser.dart 100.00% <100.00%> (ø)
...ugins/html/encoder/parser/heading_node_parser.dart 100.00% <100.00%> (ø)
.../plugins/html/encoder/parser/html_node_parser.dart 100.00% <100.00%> (ø)
...html/encoder/parser/numbered_list_node_parser.dart 100.00% <100.00%> (ø)
...plugins/html/encoder/parser/quote_node_parser.dart 100.00% <100.00%> (ø)
.../plugins/html/encoder/parser/text_node_parser.dart 100.00% <100.00%> (ø)
... and 3 more

... and 39 files with indirect coverage changes

@LucasXu0
Copy link
Collaborator

@alihassan143 Should I merge previous the PR first or these two are unrelated?

@alihassan143
Copy link
Contributor Author

@LucasXu0 this pr is different and other pr is different

@alihassan143
Copy link
Contributor Author

@you can first merged this pr other pr just need to be rebase and resolve conflicts

@alihassan143
Copy link
Contributor Author

@LucasXu0 first merge this pr than merge copy paste pr

@LucasXu0 LucasXu0 requested review from Xazin and LucasXu0 July 17, 2023 07:29
Comment on lines 260 to 263
expect(
result[4].attributes.toString(),
'''{style: font-weight: bold; font-style: italic}''',
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

the href is missing here.

@LucasXu0 LucasXu0 force-pushed the htmlparser branch 3 times, most recently from 80cdeab to bc8eb55 Compare July 19, 2023 10:43
@alihassan143
Copy link
Contributor Author

@LucasXu0
href logic implemented in that way because popular editor do the same
Lexical Editor
Tiny Editor
Quill Editor

@alihassan143
Copy link
Contributor Author

@LucasXu0 @Xazin now this pr ready to be merged

@LucasXu0
Copy link
Collaborator

@alihassan143 Please move this code to a new function.

Before

    // rich editor for webs do this so handling that case for
    //  href <a href="https://www.google.com" rel="noopener noreferrer" target="_blank"><strong><em><u>demo</u></em></strong></a>
    if (attributes[AppFlowyRichTextKeys.href] != null) {
      final element = dom.Element.tag(HTMLTags.anchor)
        ..attributes['href'] = attributes[AppFlowyRichTextKeys.href];
      dom.Element? newElement;
      dom.Element? appendElement;

      attributes.forEach((key, value) {
        if (key != AppFlowyRichTextKeys.href) {
          if (newElement == null) {
            newElement = convertSingleAttributeTextInsertToDomNode(
              text,
              {key: value},
            );
          } else {
            appendElement ??= convertSingleAttributeTextInsertToDomNode(
              "",
              {key: value},
            );

            if (appendElement != null) {
              appendElement = appendElement!..append(newElement!);
            } else {
              appendElement = convertSingleAttributeTextInsertToDomNode(
                "",
                {key: value},
              );
            }
          }
        }
      });
      if (appendElement != null) {
        element.append(appendElement!);
      } else if (newElement != null && appendElement == null) {
        element.append(newElement!);
      }

      return element;
    }

After

  dom.Element convertMultipleAttributeTextInsertToDomNode(
    String text,
    Attributes attributes,
  ) {
   // handle the href logic 
   final element = new_function_name(textInsert);
   if (element != null) {
     return element;
   }
    final span = dom.Element.tag(HTMLTags.span);
    final cssString = convertAttributesToCssStyle(attributes);
    if (cssString.isNotEmpty) {
      span.attributes['style'] = cssString;
    }
    span.append(dom.Text(text));
    return span;
}

@LucasXu0 LucasXu0 merged commit 3ee7e31 into AppFlowy-IO:main Jul 20, 2023
9 of 10 checks passed
@LucasXu0 LucasXu0 mentioned this pull request Aug 8, 2023
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.

3 participants