-
Notifications
You must be signed in to change notification settings - Fork 845
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: support for dynamic placeholders #2238
base: master
Are you sure you want to change the base?
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…uill_native_bridge
…tent and ExpandSelectionToLineBreakIntent to use keyboard shortcuts, unrelated cleanup to the bug fix. (singerdmx#2279)
…singerdmx#2332) * fix: link menu action cupertino modal popup now does not use root navigator This is to fix a bug where when using nested navigators the modal popup wouldn't pop. This also brings the behavior into parity with the Android/Material equivalent. * chore: add comment referencing bug fix singerdmx#1170 --------- Co-authored-by: Doug Todd <[email protected]> Co-authored-by: Ellet <[email protected]>
…ged to be optional
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this.
Please refer to Flutter Documentation style. Most of the old / original / legacy codebase, most packages and projects follow this style.
Documentation is important, I prefer if feature X doesn't exist if it could have poor documentation, and it's one of the main reasons we don't understand most of the codebase. I haven't done it in a good way previously but the goal is to improve the new changes.
/// so, we only need to configure `placeholderComponentsConfiguration` like: | ||
/// | ||
///```dart | ||
///final configuration = PlaceholderComponentsConfiguration( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not formatted properly, formatting is important for consistent codebase docs, and handling conflicts, not necessarily for the final output.
The general idea is to follow the Dart and Flutter style for writing docs. The lints and checks will be updated later.
/// Whether the line is empty, this let us add a placeholder | ||
/// | ||
/// _Note: these placeholders are limited only to lines with block styles_ | ||
/// | ||
/// ### Example | ||
/// | ||
/// Assume that you want to show only a placeholder text for the header items, | ||
/// so, we only need to configure `placeholderComponentsConfiguration` like: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow the Flutter/Dart style for writing docs (see example doc comment).
Try to remove the redundancy, keeping it as short as possible without details loss (not necessarily short), avoid let us
, assume that you want to ...
, we
, you
, and our
in docs.
While our current doc comments are not qualified enough, I would like to improve, not add more things to improve on the list. I will update this soon, currently, I'm fixing #2394 and other related issues in flutter_quill_extensions
(broken behavior on desktop and web platforms), once I'm done, I will handle all of the nits soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't focused on improving the documentation yet. As you said before, I will follow the documentation style you mentioned, however, my priority at the moment is to check that all the functionality is working as it should. Once I can see that everything is working correctly, I will take the time to create more documentation (not only adding doc comments but also adding files for the official documentation of the package)
lib/src/editor/raw_editor/builders/placeholder/placeholder_builder_internal.dart
Outdated
Show resolved
Hide resolved
lib/src/editor/raw_editor/builders/placeholder/placeholder_builder_internal.dart
Outdated
Show resolved
Hide resolved
lib/src/editor/raw_editor/builders/placeholder/placeholder_builder_internal.dart
Outdated
Show resolved
Hide resolved
lib/src/editor/widgets/cursor.dart
Outdated
} | ||
|
||
bool _isNodeInline(Line node) { | ||
for (final attr in node.style.attributes.values) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Responding to a comment: Let's keep it for now, this is a separate issue and there is already much to do on our list.
/// This class contains all necessary configurations | ||
/// to show the wanted placeholder at the level of the cursor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dots are not used in most of the doc comments.
…for cursorPlaceholderConfig.offset in paint method
Description
Added
placeholderConfig
parameter to allow us to build placeholders dynamically depending on what attribute we want andcursorPlaceholderConfig
that allows to the devs add a placeholder at the right (if we are using a LTR language) of the cursor (only when the paragraph and it does not contains any block attribute or selection)For example, if we only want to display placeholders for
Headers
andLists
, then we would set it up like this:Preview
Web
test_web_placeholders.mp4
Desktop
desktop_placeholders.mp4
Android
fixed_text_scale.mp4
Related Issues
N/A
TODO