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

Update TaskNodes Checkbox visual density to depend on the ThemeData.visualDensity #2444

Conversation

TahaTesser
Copy link
Contributor

@TahaTesser TahaTesser commented Dec 6, 2024

We're updating Checkbox default visual density to not depend on ThemeData.visualDensity and default to VisualDensity.standard in order to match Material Design 3 specifications in flutter/flutter#159081.

This could produce visual changes for the users, however, if the users don't want to follow M3 compliance then they can override the Checkbox.visualDensity to depend on ThemeData.visualDensity.

This PR makes such override to pass customer testing suite in the Flutter PR as super_editor is part of this suite.

@matthew-carroll
Copy link
Contributor

@TahaTesser which tests are failing without this modification?

@TahaTesser
Copy link
Contributor Author

@TahaTesser which tests are failing without this modification?

This is the test

testWidgetsOnDesktop('from list item to task', (tester) async {
await _pumpEditorWithTaskComponent(
tester,
document: MutableDocument(
nodes: [
ListItemNode.unordered(id: '1', text: AttributedText('This is a list item')),
TaskNode(id: '2', text: AttributedText('This is a task'), isComplete: false),
],
),
);
// Place the caret at "This| is a list item".
await tester.placeCaretInParagraph('1', 4);

Here is a minimal sample to reproduce the behavior.

expand to view the code sample
import 'package:flutter/material.dart';
import 'package:super_editor/super_editor.dart';

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    final composer = MutableDocumentComposer();
    final editor = createDefaultDocumentEditor(
        document: MutableDocument(
          nodes: [
            TaskNode(
                id: '1',
                text: AttributedText('This is a task'),
                isComplete: false),
            ListItemNode.unordered(
                id: '2', text: AttributedText('This is a list item')),
          ],
        ),
        composer: composer);

    return MaterialApp(
      home: Scaffold(
        body: SuperEditor(
          editor: editor,
          componentBuilders: [
            TaskComponentBuilder(editor),
            ...defaultComponentBuilders,
          ],
        ),
      ),
    );
  }
}

With flutter/flutter#159081

Screenshot 2024-12-11 at 21 32 30

With flutter/flutter#159081 and this PR

Screenshot 2024-12-11 at 21 34 10

Copy link
Contributor

@matthew-carroll matthew-carroll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Thanks.

@matthew-carroll matthew-carroll merged commit 9694523 into superlistapp:main Dec 11, 2024
12 of 14 checks passed
@matthew-carroll
Copy link
Contributor

@angelosilvestre can you cherry pick this?

@TahaTesser
Copy link
Contributor Author

TahaTesser commented Jan 2, 2025

@matthew-carroll

FYI, I'm updating flutter/tests super_editor registry to unblock Flutter PR, flutter/flutter#159081.

See flutter/tests#437. I chose the latest super_editor commit to bring this change. Feel free to drop comment in that If you've any feedback.

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

Successfully merging this pull request may close these issues.

2 participants