#46 add group management - #47
Conversation
27d735c to
af906d0
Compare
Screen.Recording.2026-06-17.at.8.04.38.AM.mov |
| /// decides what those groups are being attached to. | ||
| /// | ||
| /// Renders the currently selected groups as removable [GroupChip]s, followed by | ||
| /// an "Add" chip that opens a searchable [GroupPicker] of the offered [groups]. |
There was a problem hiding this comment.
A: "Add" is a little deceiving in this context since you can remove groups as well - "Edit"?
There was a problem hiding this comment.
✅ Good catch. The chip opens the picker, which both selects and deselects, so "Add" undersells it. Renamed the chip to "Edit" with Icons.edit_outlined, and exposed it as an editLabel parameter (default 'Edit') for i18n alongside pickerTitle/emptyHint.
| expect(controller.groupsFor('u'), isEmpty); | ||
| }); | ||
|
|
||
| test('assign ignores unknown groups', () { |
There was a problem hiding this comment.
Q: What's the logic behind ignoring vs erroring? At first blush I'd think you'd want to know the assignment failed.
There was a problem hiding this comment.
Agreed, and this is a real inconsistency — addGroup/updateGroup throw on bad ids while assign silently dropped them. A single deliberate assign to a non-existent group is a programming error, not something to swallow, so it now throws ArgumentError to match. I kept setAssignments tolerant on purpose, since it's the bulk/reconcile path where filtering stale ids is the expected behavior (documented as such). Test updated to throwsArgumentError.
| /// its mutations to their own persistence layer (REST, GraphQL, Firestore, | ||
| /// etc.) — typically by calling the controller optimistically and then | ||
| /// reconciling, or by mutating it from within a successful response handler. | ||
| class GroupManagerController extends ChangeNotifier { |
There was a problem hiding this comment.
Q: Since we use Riverpod pretty consistently, do we want this to as well?
There was a problem hiding this comment.
Intentionally framework-agnostic here. Our own apps can still wrap the controller in a provider; this just doesn't bake it in. A good example is that we typically use frozen, but as Emily points out below, maybe dart_mappable is better now? My preference would be to keep these agnostic.
| IconData? icon, | ||
| bool clearDescription = false, | ||
| bool clearColor = false, | ||
| bool clearIcon = false, |
There was a problem hiding this comment.
A: dart_mappable solves this with a sentinel object to distinguish explicit null from not-specified. A little more intuitive for the caller.
There was a problem hiding this comment.
Nice - that sentinel approach is genuinely cleaner for callers. The trade-off is that it brings a codegen/runtime dependency, and the goal for these tiny models is to stay zero-dependency and build_runner-free, so I kept the explicit clearDescription/clearColor/clearIcon flags. Happy to revisit if we ever standardize on dart_mappable across the suite.
There was a problem hiding this comment.
I was more saying we could lift the idea. Not a huge deal to me either way though.
| child: const Text('Edit'), | ||
| ), | ||
| MenuItemButton( | ||
| leadingIcon: Icon(Icons.delete_outline, color: error), |
There was a problem hiding this comment.
A: I can't find it now so maybe I made it up, but I think MD3 suggests red coloration only used for the actual confirmation button and not the trigger button to clearly signal when a button press will be irreversible.
There was a problem hiding this comment.
✅ You're right per MD3 - error coloring should be reserved for the actual irreversible action. Dropped the red tint from the Delete menu item; the confirmation dialog's Delete button keeps colorScheme.error, so red now only appears at the point of no return.
Truth in lending, I have a skill that is supposed to manage this, and have been struggling to keep it focused on the actual irreversible action. It keeps reverting to these, and I missed it.
|
Updated walkthrough with PR changes: post-pr-walkthrough.mov |
af906d0 to
35fa107
Compare
| /// its mutations to their own persistence layer (REST, GraphQL, Firestore, | ||
| /// etc.) — typically by calling the controller optimistically and then | ||
| /// reconciling, or by mutating it from within a successful response handler. | ||
| class GroupManagerController extends ChangeNotifier { |
There was a problem hiding this comment.
Q: I'm trying to imagine how you hook this up to a repository. Is the idea that we'd use the listener pattern? Does that mean the implementer is responsible for keeping the controller state in sync with the repository? How do they update controller state without re-triggering their own logic?
There was a problem hiding this comment.
On the wiring specifically: the intended pattern is persist in the action callbacks, not in a listener. The widgets hand you the user's intent — onEdit/onDelete on GroupListView, onChanged on GroupAssignmentField — and you call your repository there, then mutate the controller on success. The controller's listeners only drive UI rebuilds, so updating its state from a successful response never re-fires those callbacks — no loop, and the implementer isn't reconciling two sources of truth by hand.
The gap you spotted: GroupManagerView only exposed onTap, so the all-in-one surface had no create/edit/delete interception point — which is exactly what would have forced a listener-based save. Added onCreate/onEdit/onDelete (create/edit/delete fire the callback and suppress the default mutation), plus a "Wiring to a repository" example in the dartdoc showing both optimistic (mutate, roll back on failure) and confirm-first (await, then mutate) flows.
| onDeleted: enabled ? () => _remove(group.id) : null, | ||
| ), | ||
| if (enabled) | ||
| ActionChip( |
There was a problem hiding this comment.
A: Just IMO and IDK how achievable it is with the current design, but controls that move or are inconsistently placed based on data are difficult to use. I suppose with enabled you could customize it based on the app and you'd just lose the x delete buttons on the group chips.
There was a problem hiding this comment.
Good point - a control that drifts as chips are added/removed is hard to target. Pulled the trigger out of the chip flow into a stationary edit IconButton in the label row, so the Wrap only shows/removes members and the edit affordance never moves. (Keeps the per-chip x.)
| radius: 16, | ||
| backgroundColor: scheme.surfaceContainerHighest, | ||
| child: Icon( | ||
| selected == null ? Icons.check : Icons.auto_awesome, |
There was a problem hiding this comment.
A: Because we're marking this as the "auto" option with the avatar icon, when it is selected it's not clear that it is an auto option. Additionally, since this is the option selected by default it's very possible that a new end user would have never seen it unselected. We could overlay the auto icon on the edge of the avatar circle so it's always visible, or overlay a checkmark on the edge for the selected item.
We could also tap into our "auto" design language established with the theme picker and make the icon an "A".
There was a problem hiding this comment.
Good catch - the Auto swatch replaces its auto_awesome glyph with a plain check when selected, so in its default-selected state there's no "auto" cue and a new user may never see it unselected. I'll keep the auto identity always visible and show selection as a ring/corner check instead of swapping the glyph. I'll also align it to the "A" auto language we use in the theme picker so it reads consistently.
| _onQueryChanged(''); | ||
| }, | ||
| ), | ||
| IconButton( |
There was a problem hiding this comment.
A: Another IMO, but my read of the UI would be that the '+' is tied to the search feature, not adding a new group. We could take the empty state button for adding a new group, and just animate it shrinking down to just a plus sign button that is separate from and right of the search bar. There's also some MD argument for it being a FAB but I'm not super sold on that.
There was a problem hiding this comment.
Yeah — I've gone back and forth on a few design options here and was surprised there weren't comments on it earlier :)
I'm sold on separating it from the search bar, though - you're right that nesting the + in the trailing slot makes it read as part of search. I'll pull it out into a standalone create button to the right of the search bar. Honestly, it still looks a little weird out there, but it's slightly less confusing, and there's no great option here.
The FAB I'm going to skip: it's awkward on full-screen layouts, it's arguably not the primary action on the page, and it's especially hard for a drop-in component since owning a FAB means owning the Scaffold - which is the host app's call, not this surface's.
There was a problem hiding this comment.
Updated to:
| ? _RowMenu( | ||
| onEdit: () => | ||
| (onEdit ?? (g) => _defaultEdit(context, g))(group), | ||
| onDelete: () => |
There was a problem hiding this comment.
I: Food for thought for future iterations, a bulk delete would be nice given it's 3 clicks to delete a group. It's a rare action so probably wouldn't need to be a dominant part of the UI, but the functionality would be helpful. Even rarer, but maybe worth considering would be a bulk edit. Thinking about it though, the only thing you'd really "bulk edit" would be the icon or the color - possibly desirable but not at all critical.
There was a problem hiding this comment.
Agree - this would be a useful feature - kicking to #48
There was a problem hiding this comment.
Q/S: There is a bug in this example, and I'm having trouble thinking through whether there's some real-world case it aligns to that needs to be accounted for.
The "bug" happens when you assign a group to the signed in user (Ada), assign that group to a folder, then remove Ada from that group. The Folders tab then does not show that group in the UI for the associated folder, but does retain the assignment in the data. I.e. if you re-add Ada to the group it reappears. There is no way to remove it while Ada is not in the group.
Maybe the answer is that the implementer should have cleared that group from Ada's folders, and this is a non-issue?
There was a problem hiding this comment.
Reconciliation is the host app's policy call. Two things done: (1) documented the contract on GroupAssignmentField (keep groups a superset of selected, or reconcile when the offered set shrinks); (2) fixed the example to offer grantable ∪ already-shared, so an existing share stays visible and removable even after the signer leaves that group — you just can't add new groups you don't belong to.
71f3889 to
ccf6595
Compare
No description provided.