Extended #2528
Extended #2528
Conversation
|
Can you give us a better description of what this PR is doing? |
There was a problem hiding this comment.
This PR seems to be adding a lot of stuff without explaining anything, and just seems to bloat the spaghetti code.
Please, make sure you document what this PR does and the motivation behind it.
I'm pretty unsure if, even knowing what this PR does, we should merge it.
Yes, it doesn't confirm with the current code. For example, the button parameters are added directly without options class. I appreciate the contribution, but it shouldn't be merged, at least with the current state. |
| @@ -133,6 +136,7 @@ class Attribute<T> extends Equatable { | |||
| Attribute.list.key, | |||
| Attribute.codeBlock.key, | |||
| Attribute.blockQuote.key, | |||
| Attribute.qaBlock.key, | |||
There was a problem hiding this comment.
This doesn't confirm to Quill Delta and is not supported. It should be disabled by default or provided in an external package.
| import 'package:flutter/material.dart'; | ||
| import 'package:flutter_quill/flutter_quill.dart' as quill; | ||
| import 'package:flutter_quill/flutter_quill.dart'; | ||
|
|
||
| void main() { | ||
| runApp(MyApp()); | ||
| } | ||
|
|
||
| class MyApp extends StatelessWidget { | ||
| @override | ||
| Widget build(BuildContext context) { | ||
| final _controller = quill.QuillController.basic(); | ||
|
|
||
| return MaterialApp( | ||
| home: Scaffold( | ||
| appBar: AppBar( | ||
| title: Text('Text Editor'), | ||
| ), |
There was a problem hiding this comment.
We should have one minimal example instead of many, I don't think many will run this example as it's not clear to discover.
| @@ -78,6 +78,7 @@ class MarkdownToDelta extends Converter<String, Delta> | |||
| 'h1': (_) => [Attribute.h1], | |||
| 'h2': (_) => [Attribute.h2], | |||
| 'h3': (_) => [Attribute.h3], | |||
| 'qa-block': (_) => [Attribute.qaBlock], // Added QABlockAttribute here | |||
There was a problem hiding this comment.
The markdown package support is discounted and will be removed in future releases.
| final nodeTextDirection = getDirectionOfNode(line); | ||
| children.add( | ||
| Directionality( | ||
| textDirection: nodeTextDirection, | ||
| child: editableTextLine, | ||
| ), | ||
| ); | ||
|
|
||
| // Check if the current line is a QA Block and needs a separator |
There was a problem hiding this comment.
We shouldn't add comments that explain code that's self-explanatory unless they add something.
It doesn't make sense to have this comment, for example:
// This prints the result of 1 + 1 to the debug console
print(1 + 1)
Too many comments would bloat the code and make the code more difficult to read.
| ), | ||
| child: Column( | ||
| children: [ | ||
| QABlockSeparator(), // Custom widget for separator |
There was a problem hiding this comment.
This comment is also unnecessary.
| import 'package:flutter_quill/flutter_quill.dart'; | ||
|
|
||
| import '../../models/documents/nodes/container.dart'; | ||
| import '../others/box.dart'; // Adjust the import according to your project structure |
| feedback: Material( | ||
| child: Opacity( | ||
| opacity: 0.7, | ||
| child: child, // Display the dragged widget with some transparency |
There was a problem hiding this comment.
There are many comments that needs to be removed or adjusted.
| import '../../../../../flutter_quill.dart'; | ||
| import '../../../../models/documents/attribute.dart'; | ||
|
|
||
| class MinimalColorButton extends StatefulWidget { |
There was a problem hiding this comment.
The naming is inconsistent with the current naming. This shouldn't be part of the public API.
Description
Describe what this PR does. If modifying behavior, explain the current and new behavior, along with the motivation.
Related Issues
Type of Change