mkobuolys / flutter_deck

A lightweight, customizable, and easy-to-use framework to create presentations in Flutter.
https://pub.dev/packages/flutter_deck
MIT License
177 stars 25 forks source link

feat: Support direct slide widget creation #105

Closed matthew-carroll closed 14 hours ago

matthew-carroll commented 2 months ago

The current structural approach of flutter_deck is to subclass FlutterDeckSlideWidget to create a slide. This requirement seems to result in a lot more code than should be necessary to describe a desired slide show.

For example, I'd like to start defining my presentation with something like this:

@override
Widget build(BuildContext context) {
  return FlutterDeckApp(
    slides: [
      FlutterDeckSlide.title(title: 'The Art of Flutter Testing'),
    ],
  );
}

But the above won't compile. The slide is a FlutterDeckSlide, but the slides list wants FlutterDeckSlideWidgets.

Therefore, I have to write the following code for the exact same effect:

@override
Widget build(BuildContext context) {
  return FlutterDeckApp(
    slides: [
      const MyTitleSlide(),
    ],
  );
}

class MyTitleSlide extends FlutterDeckSlideWidget {
  const MyTitleSlide()
    : super(
        configuration: const FlutterDeckSlideConfiguration(route: '/'),
      );

  @override
  FlutterDeckSlide build(BuildContext context) {
    return FlutterDeckSlide.title(title: 'The Art of Flutter Testing');
  }
}

I'm sure there are some details behind the scenes that lead to this, but in terms of UX, I really think we should be able to directly use existing slide types within the slides list without subclassing. I know there's code generation tooling, but in this case, that seems like a solution to a problem that doesn't need to exist.

mkobuolys commented 1 month ago

Good insights. This is indeed true for simple slide templates like title, quote, big fact, etc., but this pattern could easily become cumbersome once you need to pass widget builders or anything custom. I think the subclassing is mainly needed as a convenient way to define the configuration for a slide. Having this in mind, I think it should be possible to refactor the code a bit and expose these shortcut constructors for slides' creation.

matthew-carroll commented 1 month ago

but this pattern could easily become cumbersome once you need to pass widget builders or anything custom

I already found the existing pattern to be very cumbersome. For my recent presentation, I typed a lot of code that seemed fundamentally unnecessary for the simple structure I was describing.

I think the subclassing is mainly needed as a convenient way to define the configuration for a slide

Sorry, but subclassing is anything but convenient. It's extremely verbose. And it's also confusing as to why we're extending a special type of widget, just so that we're forced to return another special type of widget.

Example of a FlutterDeckSlideWidget:

class ExampleSlide extends FlutterDeckSlideWidget {
  const ExampleSlide()
      : super(
          configuration: const FlutterDeckSlideConfiguration(
            route: '/example',
            header: FlutterDeckHeaderConfiguration(title: 'Example'),
          ),
        );

  @override
  FlutterDeckSlide build(BuildContext context) {
    return FlutterDeckSlide.blank(
      theme: FlutterDeckTheme.of(context).copyWith(
        slideTheme: const FlutterDeckSlideThemeData(
          color: Colors.white,
          backgroundColor: Colors.black87,
        ),
      ),
      builder: (context) => const Center(
        child: Text('This is an example slide'),
      ),
    );
  }
}

That widget subclass seems to exist primarily to force you to return a FlutterDeckSlide. But it's unclear why users are forced to only display FlutterDeckSlides.

Here's the implementation of FlutterDeckSlide: https://github.com/mkobuolys/flutter_deck/blob/main/packages/flutter_deck/lib/src/flutter_deck_slide.dart#L85

That implementation seems to install a few default details like a theme and a particular builder configuration. However, there's really no need to constrain users to those defaults. If a user wants to show a big red rectangle as a slide, it's not clear why a user shouldn't be allowed to simply say:

slides: [
  MySlide1(),
  ColoredBox(color: Colors.red),
  MySlide2(),
]
matthew-carroll commented 1 month ago

@mkobuolys did you have any follow up on my comments here and related comments on other issues?

I think these are some important things to iron out, because the API, in its current form, is less of a slide show toolkit, and more of a slide show template. I'm not sure that I can extend the current API approach to something more generic and compositional. If you feel strongly that you want the API to continue in it's current direction then I'm not sure I'll be able to help grow this project.

mkobuolys commented 1 month ago

@mkobuolys did you have any follow up on my comments here and related comments on other issues?

I think these are some important things to iron out, because the API, in its current form, is less of a slide show toolkit, and more of a slide show template. I'm not sure that I can extend the current API approach to something more generic and compositional. If you feel strongly that you want the API to continue in it's current direction then I'm not sure I'll be able to help grow this project.

Hey. I haven't really had time to sit down and improve flutter_deck, thus I am pretty passive with the feedback you gave - sorry about that. I agree with your point that the slide could be just a simple Widget, it makes total sense. However, I want to play with the API a bit first to see the outcome of your proposal and what side effects it could bring having the current project structure in mind.

mkobuolys commented 1 month ago

@matthew-carroll Hi Matt, I've created a POC for this issue. Could you take a look and share your opinion about it? https://github.com/mkobuolys/flutter_deck/pull/115

mkobuolys commented 14 hours ago

Implemented with https://github.com/mkobuolys/flutter_deck/pull/115