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.

Sign up for GitHub

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

Jump to bottom

Implementing switch expressions throughout the repo. #136140

Closed
wants to merge 28 commits into from
Closed

Implementing switch expressions throughout the repo. #136140

wants to merge 28 commits into from

Conversation

nate-thegrate
Copy link
Member

@nate-thegrate nate-thegrate commented Oct 7, 2023 •
edited

Example: button_theme.dart

before:

EdgeInsetsGeometry get padding {
  if (_padding != null) {
    return _padding;
  }
  switch (textTheme) {
    case ButtonTextTheme.normal:
    case ButtonTextTheme.accent:
      return const EdgeInsets.symmetric(horizontal: 16.0);
    case ButtonTextTheme.primary:
      return const EdgeInsets.symmetric(horizontal: 24.0);
  }
}

after:

EdgeInsetsGeometry get padding {
  return _padding ?? switch (textTheme) {
    ButtonTextTheme.normal || ButtonTextTheme.accent => const EdgeInsets.symmetric(horizontal: 16.0),
    ButtonTextTheme.primary => const EdgeInsets.symmetric(horizontal: 24.0),
  };
}

(solves issue #136139)


Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (N/A).
  • I updated the tests to use switch expressions as well.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. a: animation Animation APIs f: material design flutter/packages/flutter/material repository. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) f: scrolling Viewports, list views, slivers, etc. f: cupertino flutter/packages/flutter/cupertino repository d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: routes Navigator, Router, and related APIs. f: focus Focus traversal, gaining or losing focus labels Oct 7, 2023
@nate-thegrate nate-thegrate marked this pull request as draft October 7, 2023 20:42
@flutter-dashboard
Copy link

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

mateusfccp
mateusfccp reviewed Oct 9, 2023
View reviewed changes
dev/integration_tests/channels/lib/src/test_step.dart Outdated Show resolved Hide resolved
dev/integration_tests/channels/lib/src/test_step.dart Outdated Show resolved Hide resolved
dev/integration_tests/channels/lib/src/test_step.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/rendering/stack.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/rendering/viewport.dart Outdated Show resolved Hide resolved
@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. a: internationalization Supporting other languages or locales. (aka i18n) a: desktop Running on desktop f: integration_test The flutter/packages/integration_test plugin labels Oct 10, 2023
nate-thegrate and others added 3 commits October 9, 2023 18:58
@nate-thegrate @mateusfccp
Update packages/flutter/lib/src/rendering/stack.dart
ec5c64c 
Co-authored-by: Mateus Felipe C. C. Pinto <mateusfccp@gmail.com>
@nate-thegrate @mateusfccp
Update packages/flutter/lib/src/widgets/basic.dart
2f23265 
Co-authored-by: Mateus Felipe C. C. Pinto <mateusfccp@gmail.com>
@nate-thegrate
@mateusfccp's suggestion
8afd37c
@nate-thegrate nate-thegrate marked this pull request as ready for review October 11, 2023 00:35
@nate-thegrate
Copy link
Member Author

All checks have passed! 🙌

@stuartmorgan @mateusfccp would one of you be able to approve the PR?

@mateusfccp
Copy link
Contributor

All checks have passed! 🙌

@stuartmorgan @mateusfccp would one of you be able to approve the PR?

I'm only a regular contributor

mateusfccp
mateusfccp reviewed Oct 11, 2023
View reviewed changes
dev/benchmarks/platform_channels_benchmarks/lib/main.dart Outdated Show resolved Hide resolved
dev/bots/service_worker_test.dart
Comment on lines +110 to +117
ServiceWorkerTestType.blockedServiceWorkers => 'index_with_blocked_service_workers.html',
ServiceWorkerTestType.withFlutterJs => 'index_with_flutterjs.html',
ServiceWorkerTestType.withoutFlutterJs => 'index_without_flutterjs.html',
ServiceWorkerTestType.withFlutterJsShort => 'index_with_flutterjs_short.html',
ServiceWorkerTestType.withFlutterJsEntrypointLoadedEvent => 'index_with_flutterjs_entrypoint_loaded.html',
ServiceWorkerTestType.withFlutterJsTrustedTypesOn => 'index_with_flutterjs_el_tt_on.html',
ServiceWorkerTestType.withFlutterJsCustomServiceWorkerVersion => 'index_with_flutterjs_custom_sw_version.html',
ServiceWorkerTestType.generatedEntrypoint => 'generated_entrypoint.html',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this kind of alignment desired? @stuartmorgan

dev/integration_tests/channels/lib/src/basic_messaging.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/cupertino/date_picker.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/switch_list_tile.dart Show resolved Hide resolved
packages/flutter/lib/src/material/tabs.dart Show resolved Hide resolved
packages/flutter/lib/src/rendering/paragraph.dart Outdated Show resolved Hide resolved
@stuartmorgan
Copy link
Contributor

Please don't keep pinging me here; my comment was just a drive-by flagging of a style guide violation, not a review. This PR will go through the normal framework PR review process and be assigned to the right person (not me) to make a decision about making a significant style change to the framework code.

@christopherfujino
Copy link
Member

@nate-thegrate I think this change is too large to effectively review. I would recommend breaking it up into many smaller PRs (maybe 5 libraries per PR?).

@nate-thegrate nate-thegrate mentioned this pull request Nov 27, 2023
Merged
8 tasks
auto-submit bot pushed a commit that referenced this pull request Nov 28, 2023
@nate-thegrate
Implement switch expressions in dev/ (#139048)
3267fbc 
I previously made a PR (#136140) that used `switch` expressions to make some parts of the Flutter codebase easier to understand. It was assigned to the framework team, and @christopherfujino let me know that it was too large to effectively review and recommended breaking it up into smaller pull requests.

Here's a PR that only targets files in the `dev/` directory. Hopefully this will be easier to work with!

(solves issue #136139)
caseycrogers pushed a commit to caseycrogers/flutter that referenced this pull request Dec 29, 2023
@nate-thegrate @caseycrogers
Implement switch expressions in dev/ (flutter#139048)
8ed0bcc 
I previously made a PR (flutter#136140) that used `switch` expressions to make some parts of the Flutter codebase easier to understand. It was assigned to the framework team, and @christopherfujino let me know that it was too large to effectively review and recommended breaking it up into smaller pull requests.

Here's a PR that only targets files in the `dev/` directory. Hopefully this will be easier to work with!

(solves issue flutter#136139)
@nate-thegrate nate-thegrate deleted the switch-expressions branch January 26, 2024 00:32
@nate-thegrate nate-thegrate mentioned this pull request May 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuartmorgan stuartmorgan stuartmorgan left review comments

@mateusfccp mateusfccp mateusfccp left review comments

Assignees
No one assigned
Labels
a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) a: animation Animation APIs a: desktop Running on desktop a: internationalization Supporting other languages or locales. (aka i18n) a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: cupertino flutter/packages/flutter/cupertino repository f: focus Focus traversal, gaining or losing focus f: integration_test The flutter/packages/integration_test plugin f: material design flutter/packages/flutter/material repository. f: routes Navigator, Router, and related APIs. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nate-thegrate @mateusfccp @stuartmorgan @christopherfujino