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

Constify inKino #76

Merged
merged 8 commits into from
Apr 16, 2018
Merged

Constify inKino #76

merged 8 commits into from
Apr 16, 2018

Conversation

jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Apr 14, 2018

I've added const where it could be trivially swapped for new. I did indulge myself a little and swapped around widget construction in storyline_widget.dart to make more of the text widgets const.

Fixes #71

),
),
));
const captionStyle = const TextStyle(
Copy link
Owner

Choose a reason for hiding this comment

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

Now that this got a little longer, I would prefer to extract this into a new method so that it's easier to follow.

Something like:

  Widget _buildCaption() {
    ...
    if (_isExpandable) {
      content.add(_buildExpandCollapsePrompt());
    }
    ...
  }

  Widget _buildExpandCollapsePrompt() {
    const captionStyle = const TextStyle(
      fontSize: 12.0,
      fontWeight: FontWeight.w600,
      color: Colors.black54,
    );

    if (_isExpanded) {
      return const Padding(
        padding: const EdgeInsets.only(left: 4.0),
        child: const Text('[touch to collapse]', style: captionStyle),
      );
    }

    return const Padding(
      padding: const EdgeInsets.only(left: 4.0),
      child: const Text('[touch to expand]', style: captionStyle),
    );
  }

@roughike
Copy link
Owner

Thanks! I nitpicked about one tiny thing.

@roughike
Copy link
Owner

Also, is there an analysis option or similar which automatically suggests using const instead of new where appropriate?

@jonahwilliams
Copy link
Contributor Author

Yes, I've added an example analysis_options.yaml which includes the prefer_const_constructors lint. This will catch all places where you could insert a const, turns out I missed a few too.

I've also added one more option in the language setting, this is a config necessary to make flutter work with the linter. Finally there is a commented out line too: implicit-dynamic: false will help make your code dart 2 safe, since it will try and prevent you from accidentally using dynamic and passing that to something that expects a more specific type. This was generally okay in dart 1, but it's not really safe. A quick example:

void takesAMap(Map<String, String> foo) { ... }
...
Map foo = {};
takesAMap(foo); // Okay in dart 1, will fail at runtime in dart 2

@roughike
Copy link
Owner

Thanks!

The last commit actually made the Travis build fail, and it fails locally for me as well. Do you have time to either revert it or investigate how to test errors in Futures? If not, I'm happy to continue.

@jonahwilliams
Copy link
Contributor Author

That is very odd, I needed that change for the test to pass locally. Possibly due to slightly different flutter versions?

@roughike roughike merged commit 8511d22 into roughike:development Apr 16, 2018
@roughike
Copy link
Owner

Thanks a lot! Hopefully, I'll see you in I/O to thank in person!

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.

None yet

2 participants