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

Merge the 2 distinct "Like this app?" widgets into only one #5038

Closed
monsieurtanuki opened this issue Feb 4, 2024 · 10 comments · Fixed by #5086
Closed

Merge the 2 distinct "Like this app?" widgets into only one #5038

monsieurtanuki opened this issue Feb 4, 2024 · 10 comments · Fixed by #5086

Comments

@monsieurtanuki
Copy link
Contributor

What

  • We have a couple of "Like this app?" widgets, with different wordings/UI.
  • For consistency and maintenance reasons, we would be better off with only one.
  • I have a preference for the "carousel" version, with 3 possible answers (and a better UI)

Screenshot/Mockup/Before-After

The first time we log-in (successfully) (login_page.dart) Random access in carousel (smooth_product_carousel.dart)
Screenshot_1707064600 Screenshot_1707064589
@Smit56R
Copy link
Contributor

Smit56R commented Feb 10, 2024

Hey! Can I work on this issue?

@monsieurtanuki
Copy link
Contributor Author

Hey! Can I work on this issue?

@Smit56R Sure!
Basically the idea is to have a dialog (screenshot 1) that somehow looks like screenshot 2.
Easy step 1: a dialog with 3 vertical buttons with the same labels and actions as screenshot 2.
Please send a screenshot before actually sending a PR.

@Smit56R
Copy link
Contributor

Smit56R commented Feb 11, 2024

Okay, I will do that.

@g123k
Copy link
Collaborator

g123k commented Feb 13, 2024

I'm not sure this is a nice idea to merge things, as here, yes, they do the same thing, but that's not the expected behavior.

The one on the carousel is a returning message VS a one-time message for the one in the login.

@monsieurtanuki
Copy link
Contributor Author

@g123k I sort of agree with you, the "merge" wording is not correct, the goal is not a unique widget used in both cases.
My point was to use the same labels and the same 3 possible choices - which is not the case currently.
That's why I suggested a dialog with 3 vertical buttons for a quick fix.
Do we agree on that, or do you consider that the "ask me later" choice is not relevant when logging in?

@Smit56R
Copy link
Contributor

Smit56R commented Feb 13, 2024

@g123k @monsieurtanuki While you decide, I have altered the SmoothAlertDialog to have a parameter for delayAction which renders the dialogue in the proposed UI, if provided as an argument. It switches to a three-row layout or two-row layout according to the screen size.

Screenshot 1 Screenshot 2

Here is the code snippet for SmoothAlertDialog:

class SmoothAlertDialog extends StatelessWidget {
  const SmoothAlertDialog({
    this.title,
    required this.body,
    this.positiveAction,
    this.negativeAction,
    this.delayAction,
    this.actionsAxis,
    this.actionsOrder,
    this.close = false,
    this.margin,
    this.contentPadding,
  })  : assert(
          body is! LayoutBuilder,
          "LayoutBuilder isn't supported with Dialogs",
        ),
        assert(
          delayAction == null ||
              (positiveAction != null || negativeAction != null),
          "Delay action isn't supported without positive action and negative action",
        );

  final String? title;
  final bool close;
  final Widget body;
  final SmoothActionButton? positiveAction;
  final SmoothActionButton? negativeAction;
  final SmoothActionButton? delayAction;
  final Axis? actionsAxis;
  final SmoothButtonsBarOrder? actionsOrder;
  final EdgeInsets? margin;
  final EdgeInsetsDirectional? contentPadding;

  /// Default value [_defaultInsetPadding] in dialog.dart
  static const EdgeInsets defaultMargin = EdgeInsets.symmetric(
    horizontal: 40.0,
    vertical: 24.0,
  );

  static const EdgeInsetsDirectional _smallContentPadding =
      EdgeInsetsDirectional.only(
    start: SMALL_SPACE,
    top: MEDIUM_SPACE,
    end: SMALL_SPACE,
    bottom: SMALL_SPACE,
  );

  static const EdgeInsetsDirectional _contentPadding =
      EdgeInsetsDirectional.only(
    start: 22.0,
    top: VERY_LARGE_SPACE,
    end: 22.0,
    bottom: 22.0,
  );

  @override
  Widget build(BuildContext context) {
    final Widget content = _buildContent(context);
    final EdgeInsetsDirectional padding =
        contentPadding ?? defaultContentPadding(context);

    return AlertDialog(
      scrollable: false,
      elevation: 4.0,
      insetPadding: margin ?? defaultMargin,
      contentPadding: EdgeInsets.zero,
      shape: const RoundedRectangleBorder(borderRadius: ROUNDED_BORDER_RADIUS),
      content: ClipRRect(
        borderRadius: ROUNDED_BORDER_RADIUS,
        child: Scrollbar(
          child: SingleChildScrollView(
            child: Padding(
              padding: padding,
              child: Column(
                children: <Widget>[
                  content,
                  if (hasActions) _buildBottomBar(padding),
                ],
              ),
            ),
          ),
        ),
      ),
    );
  }

  Padding _buildBottomBar(EdgeInsetsDirectional padding) {
    final bool singleButton = (positiveAction != null &&
            negativeAction == null &&
            delayAction == null) ||
        (negativeAction != null &&
            positiveAction == null &&
            delayAction == null) ||
        (delayAction != null &&
            positiveAction == null &&
            negativeAction == null);

    return Padding(
      padding: EdgeInsetsDirectional.only(
        top: padding.bottom,
        start: (actionsAxis == Axis.horizontal || singleButton)
            ? SMALL_SPACE
            : 0.0,
        end: positiveAction != null &&
                negativeAction != null &&
                delayAction != null
            ? 0.0
            : SMALL_SPACE,
      ),
      child: SmoothActionButtonsBar(
        positiveAction: positiveAction,
        negativeAction: negativeAction,
        delayAction: delayAction,
        axis: actionsAxis,
        order: actionsOrder,
      ),
    );
  }

  bool get hasActions =>
      positiveAction != null || negativeAction != null || delayAction != null;

  Widget _buildContent(final BuildContext context) => DefaultTextStyle.merge(
        style: const TextStyle(height: 1.5),
        child: Column(
          mainAxisSize: MainAxisSize.min,
          children: <Widget>[
            if (title != null) _SmoothDialogTitle(label: title!, close: close),
            body,
          ],
        ),
      );

  static EdgeInsetsDirectional defaultContentPadding(BuildContext context) {
    return (context.isSmallDevice() ? _smallContentPadding : _contentPadding);
  }
}

Is this how you want it to work?

@monsieurtanuki
Copy link
Contributor Author

@Smit56R Please just add a neutralAction and display them according to the axis (vertically in this case), which will look like your first screenshot. This way the issue will be solved, the simpler the better.

@Smit56R
Copy link
Contributor

Smit56R commented Feb 15, 2024

@monsieurtanuki Okay. I will do it that way.

@Smit56R
Copy link
Contributor

Smit56R commented Feb 24, 2024

@monsieurtanuki I have completed the code. I just need clarification on a few things.

  1. How should I handle the reverse logic in the function _buildActions() inside smooth_alert_dialog.dart when all three buttons are provided?
  2. Do you want me to use the flat button for neutral and negative actions or do you want me to give them borders?

@monsieurtanuki
Copy link
Contributor Author

@Smit56R Please put the neutral button between positive and negative, and display neutral identical to the current negative button display.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants