Skip to content

Added font bold when isDefaultAction is true in CupertinoDialogAction#31308

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

Added font bold when isDefaultAction is true in CupertinoDialogAction#31308
LongCatIsLooong merged 17 commits intoflutter:masterfrom
riccardoratta:master

Conversation

@riccardoratta
Copy link
Contributor

@riccardoratta riccardoratta commented Apr 19, 2019

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 f: cupertino flutter/packages/flutter/cupertino repository label Apr 19, 2019
@LongCatIsLooong
Copy link
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
Copy link
Contributor

LongCatIsLooong commented May 2, 2019

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
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.
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`.
Copy link
Member

@xster xster left a comment

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.

@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

f: cupertino flutter/packages/flutter/cupertino repository

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants