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: support for dynamic placeholders #2238

Draft
wants to merge 112 commits into
base: master
Choose a base branch
from

Conversation

CatHood0
Copy link
Collaborator

@CatHood0 CatHood0 commented Sep 18, 2024

Description

Added placeholderConfig parameter to allow us to build placeholders dynamically depending on what attribute we want and cursorPlaceholderConfig 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 and Lists, then we would set it up like this:

configurations: QuillEditorConfigurations(
  // We can use this instance to avoid use this functionality:
  // CursorPlaceholderConfig.noPlaceholder()
  // Or, if we want to add our custom settings, we can configure manually
  // CursorPlaceholderConfig(text: "our_text'', textStyle: TextStyle(), show: true, offset: Offset(0, 2));
  // At this example case, we use this instance (just for showcases)
  cursorPlaceholderConfig: CursorPlaceholderConfig.basic(), 
  placeholderConfig:
    PlaceholderConfig(
       builders: {
         Attribute.header.key: (Attribute attr, style) {
           final values = [30, 27, 22];
           final level = attr.value as int?;
           if (level == null) return null;
           final fontSize = values[ (level - 1 < 0 || level - 1 > 3 ? 0 : level - 1)];
           return PlaceholderArguments(
             placeholderText: 'Header $level',
             style: TextStyle(fontSize: fontSize.toDouble())
                 .merge(style.copyWith(color: Colors.grey)),
             );
          },
          Attribute.list.key: (Attribute attr, style) {
            return PlaceholderArguments(
              placeholderText: 'List item',
              style: style.copyWith(color: Colors.grey),
            );
          },
      },
   ),
),

Preview

Web

test_web_placeholders.mp4

Desktop

desktop_placeholders.mp4

Android

fixed_text_scale.mp4

Related Issues

N/A

  • New feature: Adds new functionality without breaking existing features.
  • 🛠️ Bug fix: Resolves an issue without altering current behavior.
  • 🧹 Code refactor: Code restructuring that does not affect behavior.
  • Breaking change: Alters existing functionality and requires updates.
  • 🧪 Tests: Adds new tests or modifies existing tests.
  • 📝 Documentation: Updates or additions to documentation.
  • 🗑️ Chore: Routine tasks, or maintenance.
  • Build configuration change: Changes to build or deploy processes.

TODO

  • We must also make a cursor placeholder that appears at the same offset of the caret on any empty line (if the line has not block or embed attributes)
  • We must make possible add custom block attributes keys (we need to add some validations because we don't want to add any non block attribute)
  • Make this feature completely optional.
  • We need to add support for web
  • Rename classes related to placeholders (only those added by this PR).
  • Fix placeholder position on mobile devices.
  • Provide a way to calculate correctly the offset of the placeholder (only for cursor)
  • Test it on all platforms (this is an issue by itself because i don't have IOS or Mac devices)
  • Test if the feature could cause any issue if we have a line with several block attributes applied
  • Test cursor placeholder
  • Add documentation about this feature.

@CatHood0 CatHood0 added the enhancement New feature or request label Sep 18, 2024
@CatHood0 CatHood0 self-assigned this Sep 18, 2024
@CatHood0 CatHood0 marked this pull request as draft September 18, 2024 01:00
@CatHood0 CatHood0 requested a review from EchoEllet September 18, 2024 01:02
@EchoEllet

This comment was marked as off-topic.

AtlasAutocode

This comment was marked as off-topic.

@EchoEllet

This comment was marked as off-topic.

@CatHood0

This comment was marked as resolved.

@EchoEllet

This comment was marked as resolved.

EchoEllet and others added 12 commits October 28, 2024 13:53
…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]>
@EchoEllet

This comment was marked as resolved.

@CatHood0

This comment was marked as resolved.

Copy link
Collaborator

@EchoEllet EchoEllet left a 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.

example/lib/main.dart Outdated Show resolved Hide resolved
example/lib/main.dart Outdated Show resolved Hide resolved
example/lib/main.dart Outdated Show resolved Hide resolved
/// so, we only need to configure `placeholderComponentsConfiguration` like:
///
///```dart
///final configuration = PlaceholderComponentsConfiguration(
Copy link
Collaborator

@EchoEllet EchoEllet Nov 28, 2024

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.

Comment on lines 144 to 151
/// 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:
Copy link
Collaborator

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.

Copy link
Collaborator Author

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)

}

bool _isNodeInline(Line node) {
for (final attr in node.style.attributes.values) {
Copy link
Collaborator

@EchoEllet EchoEllet Nov 28, 2024

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.

Comment on lines 6 to 7
/// This class contains all necessary configurations
/// to show the wanted placeholder at the level of the cursor
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants