Skip to content

Added font bold when isDefaultAction is true in CupertinoDialogAction - #31308

Merged
LongCatIsLooong merged 17 commits into
flutter:masterfrom
riccardoratta:master
May 10, 2019
Merged

LongCatIsLooong merged 17 commits into
flutter:masterfrom
riccardoratta:master

Conversation

@riccardoratta

@riccardoratta riccardoratta commented Apr 19, 2019 •

Copy link
Copy Markdown
Contributor

Description

From the isDefaultAction doc in the CupertinoDialogAction class.

/// Set to true if button is the default choice in the dialog.
///
/// Default buttons are bold.
final bool isDefaultAction;

But actually changing this parameter doesn't cause any effect.

Example

By launching this app

import 'package:flutter/cupertino.dart';
import 'package:flutter/material.dart';

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

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: MyHomePage(),
    );
  }
}

class MyHomePage extends StatefulWidget {
  MyHomePage({Key key}) : super(key: key);

  @override
  _MyHomePageState createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  @override
  Widget build(BuildContext context) {
    return CupertinoPageScaffold(
      child: Center(
        child: CupertinoButton(
          child: Text('Alert'),
          onPressed: () {
            showDialog<void>(
                context: context,
                builder: (BuildContext context) {
                  return CupertinoAlertDialog(
                    title: Text('Title'),
                    content: Text('Content'),
                    actions: <Widget>[
                      CupertinoDialogAction(
                        child: Text('Cancel'),
                        onPressed: () => Navigator.of(context).pop(),
                      ),
                      CupertinoDialogAction(
                        child: Text('Ok'),
                        onPressed: () => Navigator.of(context).pop(),
                        isDefaultAction: true,
                      ),
                    ],
                  );
                });
          },
        ),
      ),
    );
  }
}

you will get

Screenshot 2019-04-19 at 15 17 39

where the "ok" button is not bold.

Solution

The change seem to solve the problem.

Screenshot 2019-04-19 at 15 27 09

Also isDefaultAction variable was never used in the code.

Related Issues

No related issues found.

Tests

Only performed visual test on the application (see screenshots above).

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@LongCatIsLooong LongCatIsLooong added the p: cupertino_ui cupertino_ui package in flutter/packages label Apr 19, 2019
Comment thread packages/flutter/lib/src/cupertino/dialog.dart
@LongCatIsLooong

Copy link
Copy Markdown
Contributor

Thanks for the pull request @riccardoratta!
Could you add a few tests in packages/flutter/test/cupertino/dialog_test.dart to make sure isDefaultAction works? The "Dialog destructive action styles" test in the same file looks like a good example in case you're looking for one.

@LongCatIsLooong

LongCatIsLooong commented May 2, 2019 •

Copy link
Copy Markdown
Contributor

UIAlertController.preferredAction does the same on iOS but for the same UIAlertController you can have at most one preferred action. I think it makes sense to enforce the same behavior in the constructor of CupertinoAlertDialog.

This is going to break the const constructor.

Also the name isDefaultAction might be a bit misleading for iOS developers since in the constructor it's juxtaposed with isDestructiveAction, it's easily mistakable for UIAlertAction.Style.default.

Not in the scope of this PR.

CC @xster @matthew-carroll

@matthew-carroll

Copy link
Copy Markdown
Contributor

Generally, LGTM.

@LongCatIsLooong Changing the name would be a breaking change. Is it significant enough of an issue to warrant that?

Removed `isDefaultAction` option since is not valid anymore (due to a new assert). Also performed a check on the final color of the text, that must be destructiveRed.
Comment thread packages/flutter/lib/src/cupertino/dialog.dart Outdated
Comment thread packages/flutter/test/cupertino/dialog_test.dart
Comment thread packages/flutter/test/cupertino/dialog_test.dart Outdated
Comment thread packages/flutter/lib/src/cupertino/dialog.dart Outdated
Comment thread packages/flutter/lib/src/cupertino/dialog.dart Outdated
This is the correct weight for _preferred_ action (here called default).
The alpha channel of the tested color is _removed_ since the scope of this test is only to check the output color when `isDestructiveAction` is `true`. Other tests will be performed to check the opacity (or alpha) when the action is enabled or not.
This test was wrongly removed by me because was not possible anymore to set both style properties to `true`.

@xster xster left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the tests were only testing the color in disabled state, we should add tests for enabled states too.

Otherwise LGTM.

Comment thread packages/flutter/lib/src/cupertino/dialog.dart
Comment thread packages/flutter/lib/src/cupertino/dialog.dart Outdated
@LongCatIsLooong
LongCatIsLooong merged commit 7051438 into flutter:master May 10, 2019
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

p: cupertino_ui cupertino_ui package in flutter/packages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants