[Infrastructure] Implement custom dropdown widget (Resolves #1527, Resolves #731, Resolves #689) #1538

angelosilvestre · 2023-10-20T03:16:44Z

[Infrastructure] Implement custom dropdown widget. Resolves #1527, Resolves #731, Resolves #689

Tapping at any DropdownButton in the example app toolbar causes the toolbar to immediately close. The issue is that Flutter DropdownButton displays the dropdown in its own route, which steals the focus from the editor.

This PR introduces a custom dropdown button. I called it SuperDropdownButton, but we can choose a better name. This widget displays the dropdown using an OverlayPortal. By doing so, it can share focus with SuperEditor.

We need to define how we want the public API:

What we want to make everything.
What we should do by default.

For example: What should be its default height? Do we want to make the width as large as the largest item in the list or just enough to fit the selected item?

This is how it looks so far:

Gravacao.de.Tela.2023-10-19.as.23.28.14.mov

I noticed that changing the paragraph alignment doesn't immediately rebuilds the paragraph, but it isn't related to the dropdown. I will file a ticket after this PR is merged.

Another example, constraining the dropdown height:

Gravacao.de.Tela.2023-10-19.as.23.30.06.mov

matthew-carroll · 2023-10-20T17:59:50Z

@angelosilvestre can we position the popover below the button like a more traditional popup menu? Also can we round the corners to fit better with the toolbar shape?

Generally the functionality looks good. Let's make it look the best we can for the Super Editor Example app, and then we can look at which aspects should be configurable.

matthew-carroll · 2023-10-20T18:00:50Z

super_editor/example/lib/demos/example_editor/_toolbar.dart

@@ -224,6 +224,9 @@ class _EditorToolbarState extends State<EditorToolbar> {
        ),
      ]);
    }
+
+    // Rebuild to display the selected item.
+    setState(() {});


Where is the selected item stored? Why do we want setState() to be empty?

The dropdown itself doesn't store the selected value, it's always provided via constructor.

In the toolbar, this value also isn't stored, we query the node type from the selected node when we create the dropdown.

I don't understand those explanations. If the selected item isn't stored, then why are calling setState()?

I'm calling setState to rebuild the toolbar. Then, it will query the value again.

I still don't understand. Please try to explain in terms of why this is needed. On the face of it, I don't know why you would need to call setState() to rebuild a widget tree that internally is using some value that changed elsewhere. Because there isn't much explanation here, I'm not even sure what to ask, but for example, why are we not using a ValueListenable somewhere, etc?

After taking a better look at this, I think that the main problem here is that we aren't rebuilding the toolbar when the document changes.

After we change the selected node's metadata, we need to rebuild the toolbar so it can "see" the current state of the selected node.

I moved this setState call to the document change listener. Please let me know if this is an acceptable solution.

super_editor/example/lib/demos/example_editor/_toolbar.dart

matthew-carroll · 2023-10-20T18:04:03Z

super_editor/lib/src/infrastructure/dropdown.dart

+    this.dropdownKey,
+  });
+
+  /// The items that will be displayed in the dropdown list.


Let's re-order these properties to match our typical ordering.

Re-ordered to match how we are using it on _toolbar.dart

super_editor/lib/src/infrastructure/dropdown.dart

matthew-carroll · 2023-10-20T18:05:59Z

super_editor/lib/src/infrastructure/dropdown.dart

+/// when the dropdown is visible.
+///
+/// This widget doesn't enforce any style, dropdown position or decoration.
+class RawDropdown extends StatefulWidget {


What does "Raw" mean here?

It only mean that we don't make any decisions here about how the widget should look like. We could probably find a better name for it... UnstyledDropdown, OverlayDropdown, or something else.

Can you elaborate on that explanation? You've said what this widget "isn't" doing. Can you describe what it "is" doing?

I tried to summarize what this widget does in the previous lines:

It listens for changes of its OverlayPortalController and shows/hides the Overlay.

Setup focus sharing.

Requests focus to the dropdown's FocusNode when the overlay is displayed.

Delivers key events to onKeyEvent.

Closes the dropdown when tapping outside of it.

I'm confused why the follower and aligner are configured elsewhere instead of here? If this widget does what you describe, why wouldn't it also include positioning?

Is this widget expected to be used publicy? If not, can we make it private?

If we make it private and move the follower and aligner in here, let's call this _FollowerPopover. The content of the popover can then be provided via child to decide what to display within the popover.

I've made it that way so we (or others) could re-use the RawDropDown to implement other kinds of dropdown buttons, which could have different following policies.

For example, it would be relatively easy to implement something like a simple (and incomplete) color picker with the following code:

class ColorPicker extends StatefulWidget { const ColorPicker({super.key}); @override State<ColorPicker> createState() => _ColorPickerState(); } class _ColorPickerState extends State<ColorPicker> { final DropdownController _dropdownController = DropdownController(); // This might be removed. final FocusNode _focusNode = FocusNode(); final List<MaterialColor> colors = [ Colors.red, Colors.blue, Colors.amber, Colors.brown, Colors.green, Colors.orange, Colors.pink, Colors.teal ]; @override void dispose() { _dropdownController.dispose(); _focusNode.dispose(); super.dispose(); } @override Widget build(BuildContext context) { return RawDropdown( controller: _dropdownController, dropdownBuilder: _buildColorPopover, parentFocusNode: _focusNode, child: ElevatedButton( onPressed: () => _dropdownController.open(), child: Text("Pick a color"), ), ); } Widget _buildColorPopover(BuildContext context, LeaderLink link, FollowerBoundary boundary) { return Follower.withOffset( link: link, leaderAnchor: Alignment.bottomCenter, followerAnchor: Alignment.topCenter, offset: Offset(0, 10), child: Material( elevation: 4.0, borderRadius: BorderRadius.circular(8.0), child: Container( height: 50, width: 200, padding: const EdgeInsets.all(8.0), child: ListView.separated( scrollDirection: Axis.horizontal, // TODO: implement selection itemBuilder: (context, index) => Container( decoration: BoxDecoration( shape: BoxShape.circle, color: colors[index], ), height: 20, width: 20, ), separatorBuilder: (context, index) => SizedBox(width: 10), itemCount: colors.length, ), ), ), ); } }

Gravacao.de.Tela.2023-11-02.as.11.46.47.mov

super_editor/test/infrastructure/dropdown_test.dart

angelosilvestre · 2023-10-21T12:39:18Z

@angelosilvestre can we position the popover below the button like a more traditional popup menu? Also can we round the corners to fit better with the toolbar shape?

@matthew-carroll Yes, initially I left the dropdown centered to match the material dropdown.

Updated:

Gravacao.de.Tela.2023-10-21.as.09.34.45.mov

With slow animations:

Gravacao.de.Tela.2023-10-21.as.09.35.31.mov

matthew-carroll · 2023-10-22T23:55:00Z

@angelosilvestre I see in your latest video that the popover prevents itself from moving beyond the bottom of the screen, but that causes it to cover the toolbar. My first thought is that this is unexpected and we don't want it. Was there any particular reason that you went with a screen-constrained popup?

The following heuristic comes to mind:

When there's enough room beneath the toolbar, show the popup below the toolbar, otherwise
When there's enough room above the toolbar, show the popup above the toolbar, otherwise
If there's a reasonable amount of space (enough to see a few items), make the popover shorter and re-apply above rules, otherwise
Pin the popup in the standard position and size below the button and let it go off-screen

angelosilvestre · 2023-10-27T00:59:26Z

@matthew-carroll As discussed, I implemented a positioning behavior similar to Notion's.

If there is enough room to display the dropdown list beneath the toolbar, position it below the toolbar.
If there is enough room to display the dropdown list above the toolbar, position it above the toolbar.
Pin the dropdown list to the bottom and let it cover the toolbar.

I also simplified the transition to be just a fade-in animation.

This is how it's looking right now:

Gravacao.de.Tela.2023-10-26.as.21.50.39.mov

For reference, this is how Notion's dropdown looks:

Gravacao.de.Tela.2023-10-26.as.21.54.26.mov

matthew-carroll · 2023-10-29T23:54:50Z

super_editor/example/lib/demos/example_editor/_toolbar.dart

@@ -224,6 +224,9 @@ class _EditorToolbarState extends State<EditorToolbar> {
        ),
      ]);
    }
+
+    // Rebuild to display the selected item.
+    setState(() {});


I still don't understand. Please try to explain in terms of why this is needed. On the face of it, I don't know why you would need to call setState() to rebuild a widget tree that internally is using some value that changed elsewhere. Because there isn't much explanation here, I'm not even sure what to ask, but for example, why are we not using a ValueListenable somewhere, etc?

super_editor/example/lib/demos/example_editor/_toolbar.dart

super_editor/lib/src/infrastructure/dropdown.dart

matthew-carroll · 2023-10-30T00:03:21Z

super_editor/lib/src/infrastructure/dropdown.dart

+/// with the [parentFocusNode]. This means that when the dropdown requests focus, [parentFocusNode]
+/// still has non-primary focus.
+///
+/// The dropdown tries to fit all items on the available space. If there isn't enough room,


Let's simply enumerate the rules for clarity:

/// The popover list is positioned based on the following rules: /// /// 1. The popover is displayed below the selected item, if there's enough room, or /// 2. The popover is displayed above the selected item, if there's enough room, or /// 3. The popover is displayed with its bottom aligned with the bottom of /// the given boundary, and it covers the selected item.

I didn't mention scrolling constraints in there. Please update accordingly. The goal is to make the rules quick and easy to understand.

I'm not sure I understood what "scrolling constraints" means here.

I'm not entirely sure what I meant, but I think I may have been referring to the content in the popover going from fully-visible to partially visible + scrolling. For example, let's say we pull up a list of options in a popover. There's likely some kind of decision whether to make the popover as tall as all those items in the list, or whether to make the popover shorter, and introduce scrolling to the list. That choice about adding scrolling, or not, and the resulting height, impacts all of the rules above. They're like two sides of the same coin.

super_editor/lib/src/infrastructure/dropdown.dart

matthew-carroll · 2023-10-30T00:30:33Z

super_editor/lib/src/infrastructure/dropdown.dart

+    int? newFocusedIndex;
+    if (event.logicalKey == LogicalKeyboardKey.arrowDown) {
+      if (_focusedIndex == null || _focusedIndex! >= widget.items.length - 1) {
+        // We don't have a focused item or we are at the end of the list. Focus the first item.


What does it mean to not have a focused item? Is that not an error state?

In the current implementation, the popover list opens without an "active item". The active item is set only after the user presses ARROW DOWN or ARROW UP.

This is compatible with other popover list implementations.

Really? If we're looking at a typical address form on the web, and there's a country selector, and we have United States selected, when we open that selector and show the popover, "United States" isn't active? And the popover isn't centered on the "United States" item?

On a related note, if no item is active initially, which one becomes active when you press UP or DOWN?

Really? If we're looking at a typical address form on the web, and there's a country selector, and we have United States selected, when we open that selector and show the popover, "United States" isn't active? And the popover isn't centered on the "United States" item?

It makes sense. Updated to make the selected value "active" in the list.

On a related note, if no item is active initially, which one becomes active when you press UP or DOWN?

Pressing UP makes the last item active and pressing DOWN makes the first item active. We could also change the behavior to make the first item active if there isn't a selected item.

matthew-carroll · 2023-10-30T00:31:15Z

super_editor/lib/src/infrastructure/dropdown.dart

+      parentFocusNode: widget.parentFocusNode,
+      onKeyEvent: _onKeyEvent,
+      child: ConstrainedBox(
+        // TODO: what value should we use?


When I first read this, I thought this was the popover list constraint. But this is constraining the selected item display, right? If so, why are we constraining that at all?

I don't remember why I added that, but it seems it's unnecessary. Removed.

matthew-carroll · 2023-10-30T00:33:11Z

super_editor/lib/src/infrastructure/dropdown.dart

+  }
+
+  Widget _buildSelectedItem() {
+    return Row(


Should this be a Row or a Stack? Imagine the developer wants to display a selected item with their own background color or gradient, how would they get a full-bleed display if we're pushing their widget to the side for a dropdown arrow?

Updated it to be a Stack.

matthew-carroll · 2023-10-30T00:37:54Z

super_editor/lib/src/infrastructure/dropdown.dart

+/// when the dropdown is visible.
+///
+/// This widget doesn't enforce any style, dropdown position or decoration.
+class RawDropdown extends StatefulWidget {


I'm confused why the follower and aligner are configured elsewhere instead of here? If this widget does what you describe, why wouldn't it also include positioning?

Is this widget expected to be used publicy? If not, can we make it private?

If we make it private and move the follower and aligner in here, let's call this _FollowerPopover. The content of the popover can then be provided via child to decide what to display within the popover.

angelosilvestre · 2023-11-02T15:22:13Z

@matthew-carroll I made some changes based on your comments, and replied some of them with other questions.

matthew-carroll · 2023-11-02T21:39:31Z

super_editor/example/lib/demos/example_editor/_toolbar.dart

+    // The document has changed.
+    // Some of the selected node's metadata, for example, the blockType, influences how the toolbar is displayed.
+    // Reflow the toolbar to acount for the new state of the selected node.
+    setState(() {});


Is there any particular reason that we don't listen for this change around, or within the pieces of the toolbar that care, rather than rebuilding the whole toolbar?

matthew-carroll · 2023-11-02T21:42:40Z

super_editor/example/lib/demos/example_editor/_toolbar.dart

@@ -521,26 +541,30 @@ class _EditorToolbarState extends State<EditorToolbar> {
              if (_isConvertibleNode()) ...[
                Tooltip(
                  message: AppLocalizations.of(context)!.labelTextBlockType,
-                  child: DropdownButton<_TextType>(
+                  child: ItemSelector<_TextType>(


I'm wondering if there's a more useful way to breakdown responsibilities for these widgets. After you go through the other new comments, let's jump on a call and talk about how to break things up.

My gut feeling is that we want two different levels of widgets here. We want foundational pieces that make almost zero visual designs but provide some base level relationships. Then we want some widgets that give us default styling, and also take a few properties to configure obvious UI details. At the moment, we're trying to do both of those things in one group of widgets.

It makes sense, we can jump on a call to talk about that.

rutvik110 · 2023-11-05T06:24:56Z

I noticed that changing the paragraph alignment doesn't immediately rebuilds the paragraph, but it isn't related to the dropdown. I will file a ticket after this PR is merged.

@angelosilvestre I'm taking look at it as part of the #1407

angelosilvestre · 2023-11-11T17:52:40Z

@matthew-carroll As discussed, the API is changing. Some things are a bit different from what we've talked. Here's the summary:

We have now the following widgets:

`PopoverScaffold` (still needs a better name), which takes:

A PopoverController that opens and closes the popover.
A buttonBuilder to build the button.
A popoverBuilder to build the popover.
A popoverGeometry to control the size and position of the popover.
An onKeyEvent to which key events will be forwarded. This widget uses a SuperEditorPopover to setup the focus sharing, and SuperEditorPopover is the widget which install the Focus widget on the tree. Because of that, instead of adding a Focus ourselves, we just forward the onKeyEvent.
An optional parentFocusNode to share focus
An optional boundaryKey to keep the popover within the boundary.

This widget doesn't take a list of items because it doesn't care about whats being displayed on the popover. We could use the popover to display a text, an image preview, etc.

`ItemSelectionList`, which takes:

The selected value.
The list of items.
A buttonBuilder to build the button.
An itemBuilder to build the each item on the list.
An onItemActivated to notify when an item is activated.
An onItemSelected to notify when an item is selected.
A popoverGeometry to control the size and position of the popover.
An optional parentFocusNode to share focus.
An optional boundaryKey to keep the popover within the boundary.

This widget handles the keyboard navigation, auto-scrolling and builds the list of options. I ignored popovers with horizontal scrolling for now. I'm not sure if we need a widget in between PopoverScaffold and ItemSelectionList, which instead of an itemBuilder would take a popoverBuilder to build the entire popover based on the list of items.

`SuperEditorDemoTextItemSelector` and `SuperEditorDemoIconItemSelector`:

Widgets with a predefined style to be used in the demos.

angelosilvestre · 2023-11-11T17:56:15Z

Rebased to fix an import.

matthew-carroll · 2023-11-13T20:15:01Z

@angelosilvestre can you please edit your comment up above to point out what you changed from our discussion and why?

Based on what I'm seeing in that comment right now, here are a couple comments:

PopoverScaffold

An onKeyEvent to which key events will be forwarded. This widget uses a SuperEditorPopover to setup the focus sharing, and SuperEditorPopover is the widget which install the Focus widget on the tree. Because of that, instead of adding a Focus ourselves, we just forward the onKeyEvent.

What do you mean "This widget uses a SuperEditorPopover"? The scaffold is the lowest level widget here and SuperEditorPopover is the highest level. If the scaffold is truly using a SuperEditorPopover, then that needs to be reversed.

"instead of adding a Focus ourselves" - I'm not sure what that means either, but you said the scaffold takes a parentFocusNode, which indicates that the scaffold knows about and cares about focus. Unless I'm missing something, we don't want to split focus responsibilities across multiple widgets.

ItemSelectionList

Why is this list taking a geometry? I believe the purpose of this widget, as discussed on our call, was to provide a list widget. That list widget might be placed within a popover, but its not, itself, a popover. It's a list that understands the concept of "selection" and "activation" and arrow key navigation, and ENTER to select the activated item, and auto-scrolling when pressing arrows keys near the top and bottom of the visible space. All of that is independent from use within a popover. Such a widget would be equally applicable for a selection list that's placed within a typical screen UI.

Please let me know if I'm missing something.

angelosilvestre · 2023-11-13T22:12:25Z

What do you mean "This widget uses a SuperEditorPopover"? The scaffold is the lowest level widget here and SuperEditorPopover is the highest level. If the scaffold is truly using a SuperEditorPopover, then that needs to be reversed.

Maybe we have some naming issue... SuperEditorPopover is a widget introduce in another PR. It sets up the focus sharing and also add the blocked intents needed for macOS. It's this widget:

super_editor/super_editor/lib/src/infrastructure/popovers.dart

Lines 5 to 64 in 6e2040f

    
           /// A popover that shares focus with a `SuperEditor`. 
        
           /// 
        
           /// Popovers often need to handle keyboard input, such as arrow keys for 
        
           /// selection movement. But `SuperEditor` also needs to continue handling 
        
           /// keyboard input to move the caret, enter text, etc. In such a case, the 
        
           /// popover has primary focus, and `SuperEditor` has non-primary focus. 
        
           /// Due to the way that Flutter's `Actions` system works, along with 
        
           /// Flutter's default response to certain key events, a few careful 
        
           /// adjustments need to be made so that a popover works with 
        
           /// `SuperEditor` as expected. This widget handles those adjustments. 
        
           /// 
        
           /// Despite this widget being a "Super Editor popover", this widget can be 
        
           /// placed anywhere in the widget tree, so long as it's able to share focus 
        
           /// with `SuperEditor`. 
        
           /// 
        
           /// This widget is purely logical - it doesn't impose any particular layout 
        
           /// or constraints. It's up to you whether this widget tightly hugs your 
        
           /// popover [child], or whether it expands to fill a space. 
        
           /// 
        
           /// It's possible to create a `SuperEditor` popover without this widget. 
        
           /// This widget doesn't have any special access to `SuperEditor` 
        
           /// properties or behavior. But, if you choose to display a popover 
        
           /// without using this widget, you'll likely need to re-implement this 
        
           /// behavior to avoid unexpected user interaction results. 
        
           class SuperEditorPopover extends StatelessWidget { 
        
             const SuperEditorPopover({ 
        
               super.key, 
        
               required this.popoverFocusNode, 
        
               required this.editorFocusNode, 
        
               this.onKeyEvent, 
        
               required this.child, 
        
             }); 
        
             /// The [FocusNode] attached to the popover. 
        
             final FocusNode popoverFocusNode; 
        
             /// The [FocusNode] attached to the editor. 
        
             /// 
        
             /// The [popoverFocusNode] will be reparented with this [FocusNode]. 
        
             final FocusNode editorFocusNode; 
        
             /// Callback that notifies key events. 
        
             final FocusOnKeyEventCallback? onKeyEvent; 
        
             /// The popover to display. 
        
             final Widget child; 
        
             @override 
        
             Widget build(BuildContext context) { 
        
               return Actions( 
        
                 actions: disabledMacIntents, 
        
                 child: FocusWithCustomParent( 
        
                   focusNode: popoverFocusNode, 
        
                   parentFocusNode: editorFocusNode, 
        
                   onKeyEvent: onKeyEvent, 
        
                   child: child, 
        
                 ), 
        
               ); 
        
             } 
        
           }

Why is this list taking a geometry? I believe the purpose of this widget, as discussed on our call, was to provide a list widget. That list widget might be placed within a popover, but its not, itself, a popover. It's a list that understands the concept of "selection" and "activation" and arrow key navigation, and ENTER to select the activated item, and auto-scrolling when pressing arrows keys near the top and bottom of the visible space. All of that is independent from use within a popover. Such a widget would be equally applicable for a selection list that's placed within a typical screen UI.

I think I misunderstood it a little bit. I'll update it.

angelosilvestre · 2023-11-14T00:20:18Z

@matthew-carroll Updated the PR. I think now it's closer to what we've talked.

I temporarily commented the test until we are sure the API is right.

Can you do a first pass review?

matthew-carroll

I did an initial review. One big initial change should be to move all the demo stuff into the demo app, instead of in the package. That might clarify some of the relationships and also point out where some things are named poorly.

super_editor/example/lib/demos/example_editor/_toolbar.dart

matthew-carroll · 2023-11-14T23:56:34Z

super_editor/example/lib/demos/example_editor/_toolbar.dart

-                              child: Padding(
-                                padding: const EdgeInsets.only(left: 16.0),
-                                child: Text(_getTextTypeName(textType)),
+                  child: _DocumentListenableBuilder(


Why do we need the document listenable?

With the recent changes we don't need it anymore.

super_editor/example/lib/demos/example_editor/_toolbar.dart

super_editor/lib/src/infrastructure/dropdown.dart

matthew-carroll · 2023-11-15T00:24:33Z

super_editor/lib/src/infrastructure/dropdown.dart

+  }
+}
+
+/// A rounded rectangle shape with a fade-in transition.


I don't think we should combine the shape with the transition animation. Those don't seem related. Also, this popover shape doesn't allow any configuration for the shape...

I'm currently using a Material widget to build the popover shape, should I take a ShapeBorder to customize the popover shape?

I'm confused - how does the use of Material relate to the two issues I raised above?

The Material widget is related just with the second sentence:

Also, this popover shape doesn't allow any configuration for the shape...

Sure you can take a BorderShape if that makes sense. But first, let's figure out what the division should look like between the shape and the transition.

If we split the transition from the shape, then the PopoverShape widget won't bring much value, because its implementation will be just:

Widget build(BuildContext context) { return Material( elevation: 8, borderRadius: BorderRadius.circular(12), clipBehavior: Clip.hardEdge, child: child ); }

super_editor/lib/src/infrastructure/dropdown.dart

matthew-carroll · 2023-11-15T00:30:44Z

super_editor/lib/src/infrastructure/dropdown.dart

+  }
+}
+
+/// Controls the size and position of a popover.


This data structure doesn't control anything. This data structure is the offset and size of a popover in screen space. Please try to avoid the term "position" when you're not talking about text or document positions.

super_editor/lib/src/infrastructure/dropdown.dart

Test Update tests Update Focus color Animation Animation

angelosilvestre · 2023-12-13T19:38:23Z

@matthew-carroll Added tests

super_editor/lib/src/infrastructure/selectable_list.dart

matthew-carroll · 2023-12-14T00:42:12Z

super_editor/lib/src/infrastructure/selectable_list.dart

+
+/// Builds a popover list item.
+///
+/// [isActive] is `true` if [item] is the currently active item on the list, or `false` otherwise.


It's probably a good idea to say something about what it means to be active, e.g., the item in the list that currently has focus and can be moved up/down with arrow keys.

matthew-carroll · 2023-12-14T00:46:19Z

super_editor/test/infrastructure/popover_test.dart

+      expect(find.byType(RoundedRectanglePopoverAppearance), findsNothing);
+    });
+
+    testWidgetsOnAllPlatforms('enforces the given popover geometry', (tester) async {


If this is only testing height constraints, then please name the test accordingly. This name suggests that all aspects of geometry are being verified against all possible conditions.

Related to that, is testing just the height of the popover, in just one set of conditions, sufficient to lock down the geometry implementation?

I'm still confused about why a single test is sufficient for this. Don't we have rules about where the popover appears based on surrounding space, and how large the popover is allowed to be? If so, then it seems like we should have a thorough collection of tests to lock down each of those behaviors...

Ok, this test is to ensure we honor an app-configured geometry. We don't have tests for our default geometry. I'll add those.

super_editor/test/infrastructure/popover_test.dart

matthew-carroll · 2023-12-14T00:47:52Z

super_editor/test/infrastructure/popover_test.dart

+      expect(tester.getRect(find.byType(RoundedRectanglePopoverAppearance)).height, 10);
+    });
+
+    testWidgetsOnAllPlatforms('shares focus with SuperEditor', (tester) async {


This test seems overly specific. Do we actually care about the popover sharing focus with SuperEditor? Or do we care about sharing focus with any arbitrary widget that wants to share?

super_editor/test/infrastructure/popover_test.dart

matthew-carroll · 2023-12-21T19:15:43Z

@angelosilvestre wanted to make sure you didn't forget about this PR.

angelosilvestre · 2023-12-21T22:40:02Z

@matthew-carroll Updated the PR

angelosilvestre · 2023-12-24T12:11:08Z

@matthew-carroll Added teste for the default popover aligner.

matthew-carroll · 2023-12-24T20:35:43Z

super_editor/test/infrastructure/popover_test.dart

+          expect(popoverRect.top, greaterThan(buttonRect.bottom));
+        });
+
+        testWidgetsOnAllPlatforms('positions the popover above button if there is room', (tester) async {


Can you adjust the test descriptions to make it clear that there are competing locations? If you read the test above this one, and then you read this test, there's no indication about why one might happen instead of the other. All we know is that if there's room below, it goes below, and if there's room above, it goes above, which begs the question, what if there's room below and above?

super_editor/test/infrastructure/popover_test.dart

matthew-carroll · 2023-12-24T20:36:49Z

super_editor/test/infrastructure/popover_test.dart

+          final popoverRect = tester.getRect(find.byType(RoundedRectanglePopoverAppearance));
+
+          // Ensure popover was pinned of the bottom to the boundary widget.
+          expect(popoverRect.bottom, 600);


Do we care about the size in this situation? Is the size impacted by the fact that there's limited room?

Yeah, updated.

matthew-carroll · 2023-12-24T20:37:52Z

super_editor/test/infrastructure/popover_test.dart

+        });
+
+        testWidgetsOnAllPlatforms(
+            'pins the popover to the bottom if there isn\'t room neither below or above the button', (tester) async {


All of these test descriptions for "without a boundary" are the same as the ones with a boundary, which leads me to believe there's no difference in behavior whether we use a boundary or not. Shouldn't the boundary do something?

I modified the tests to use a different screen size and different widget sizes with a comment explaining why.

matthew-carroll · 2023-12-24T20:38:51Z

super_editor/test/infrastructure/popover_test.dart

+      });
+    });
+
+    testWidgetsOnAllPlatforms('shares focus with other widgets', (tester) async {


Let's be much more specific about the feature this is locking down. It's not simply that it shares focus with other widgets - all widgets share focus with other widgets. Please describe this test in terms of the special situation we've implemented for this popover system.

matthew-carroll · 2023-12-28T06:45:46Z

super_editor/test/infrastructure/popover_test.dart

+          expect(popoverRect.bottom, lessThan(buttonRect.top));
+        });
+
+        testWidgetsOnAllPlatforms('pins the popover to the bottom if there is not room below or above the button',


Can you clarify "to the bottom" of what? screen? boundary? button?

matthew-carroll · 2023-12-28T20:35:13Z

In addition to earlier comments, I checked out this branch so I could work on paragraph alignment. I discovered that we're displaying too many alignment options in the drop down. This is because we're looping through all TextAlign values which means we're showing both left and start, and right and end. So we're essentially repeating equivalent options.

items: TextAlign.values
          .map(
            (alignment) => SuperEditorDemoIconItem(
              icon: _buildTextAlignIcon(alignment),
              id: alignment.name,
            ),
          )
          .toList(),

matthew-carroll · 2023-12-28T20:55:08Z

@angelosilvestre I'm gonna merge this so that I can fix paragraph alignment, which I based off of this branch so that I could select the paragarph alignment from the toolbar. Please put up a followup PR with the final adjustments from the review.

matthew-carroll · 2023-12-28T20:55:34Z

Also, please cherry pick this to stable, per usual.

…app#1527, Resolves superlistapp#731, Resolves superlistapp#689) (superlistapp#1538)

…solves #731, Resolves #689) (#1538)

angelosilvestre requested a review from matthew-carroll October 20, 2023 03:17

matthew-carroll reviewed Oct 20, 2023

View reviewed changes

angelosilvestre requested a review from matthew-carroll October 21, 2023 12:39

matthew-carroll reviewed Oct 30, 2023

View reviewed changes

angelosilvestre requested a review from matthew-carroll November 2, 2023 15:22

matthew-carroll reviewed Nov 2, 2023

View reviewed changes

angelosilvestre force-pushed the 1527_custom_dropdown branch from b50ddf9 to 4b5e83d Compare November 11, 2023 17:55

angelosilvestre requested a review from matthew-carroll November 11, 2023 18:00

rutvik110 mentioned this pull request Nov 14, 2023

Adds paragraph alignment request and command (Resolves #1587) #1594

Closed

matthew-carroll reviewed Nov 15, 2023

View reviewed changes

angelosilvestre added 9 commits December 4, 2023 09:29

Implement the dropdown widget

b9fbd90

Test Update tests Update Focus color Animation Animation

PR updates

9862741

Change dropdown alignment

7ecbeb5

Simplify animation

e0ebe87

Remove unused import

d1b1376

Rename the dropdown widget and update docs

e3f77b1

PR Updates

fb7fffc

Remove required boundaryKey

96eba09

Remove constrained box

39a5a55

Add tests

9d2a42f

angelosilvestre requested a review from matthew-carroll December 13, 2023 19:38

matthew-carroll reviewed Dec 14, 2023

View reviewed changes

angelosilvestre added 2 commits December 21, 2023 19:31

Test updates

6de054d

Split test files

d986e6a

angelosilvestre requested a review from matthew-carroll December 21, 2023 22:39

Add tests for default popover geometry

e306b88

matthew-carroll reviewed Dec 24, 2023

View reviewed changes

angelosilvestre added 2 commits December 26, 2023 19:41

Fix popover size when using boundary key

5bd8ff9

Update tests

724dd1a

angelosilvestre requested a review from matthew-carroll December 26, 2023 22:52

matthew-carroll reviewed Dec 28, 2023

View reviewed changes

matthew-carroll merged commit 509007f into superlistapp:main Dec 28, 2023
6 of 11 checks passed

angelosilvestre deleted the 1527_custom_dropdown branch December 28, 2023 22:10

angelosilvestre added a commit to angelosilvestre/super_editor that referenced this pull request Dec 28, 2023

[Infrastructure] Implement custom dropdown widget (Resolves superlist…

ff82213

…app#1527, Resolves superlistapp#731, Resolves superlistapp#689) (superlistapp#1538)

angelosilvestre mentioned this pull request Dec 28, 2023

Cherry-pick "[Infrastructure] Implement custom dropdown widget (Resolves #1527, Resolves #731, Resolves #689) (#1538)" to stable #1715

Merged

angelosilvestre added a commit to angelosilvestre/super_editor that referenced this pull request Dec 28, 2023

Updates for superlistapp#1538

0dc8496

matthew-carroll pushed a commit that referenced this pull request Dec 28, 2023

[Infrastructure] Implement custom dropdown widget (Resolves #1527, Re…

b55a5c6

…solves #731, Resolves #689) (#1538)

angelosilvestre mentioned this pull request Dec 28, 2023

Updates for #1538 #1717

Merged

matthew-carroll pushed a commit that referenced this pull request Dec 28, 2023

Updates for #1538 (#1717)

c176b32

angelosilvestre added a commit to angelosilvestre/super_editor that referenced this pull request Dec 28, 2023

Updates for superlistapp#1538 (superlistapp#1717)

6bcde02

angelosilvestre mentioned this pull request Dec 28, 2023

Cherry-pick "Updates for #1538 (#1717)" to stable #1719

Merged

matthew-carroll pushed a commit that referenced this pull request Dec 28, 2023

Updates for #1538 (#1717)

0147723

[Infrastructure] Implement custom dropdown widget (Resolves #1527, Resolves #731, Resolves #689) #1538

[Infrastructure] Implement custom dropdown widget (Resolves #1527, Resolves #731, Resolves #689) #1538

Conversation

angelosilvestre commented Oct 20, 2023

matthew-carroll commented Oct 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelosilvestre Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

angelosilvestre commented Oct 21, 2023

matthew-carroll commented Oct 22, 2023

angelosilvestre commented Oct 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelosilvestre commented Nov 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rutvik110 commented Nov 5, 2023

angelosilvestre commented Nov 11, 2023

PopoverScaffold (still needs a better name), which takes:

ItemSelectionList, which takes:

SuperEditorDemoTextItemSelector and SuperEditorDemoIconItemSelector:

angelosilvestre commented Nov 11, 2023

matthew-carroll commented Nov 13, 2023

PopoverScaffold

ItemSelectionList

angelosilvestre commented Nov 13, 2023

angelosilvestre commented Nov 14, 2023

matthew-carroll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelosilvestre commented Dec 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-carroll commented Dec 21, 2023

angelosilvestre commented Dec 21, 2023

angelosilvestre commented Dec 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelosilvestre Nov 2, 2023 •

edited

Loading

`PopoverScaffold` (still needs a better name), which takes:

`ItemSelectionList`, which takes:

`SuperEditorDemoTextItemSelector` and `SuperEditorDemoIconItemSelector`: