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

Implement /flutter for flutter.new as firebase redirect #3093

Merged
merged 4 commits into from
Nov 20, 2024
Merged

Implement /flutter for flutter.new as firebase redirect #3093

merged 4 commits into from
Nov 20, 2024

Conversation

parlough
Copy link
Member

This removes the concept of a "default snippet" from the samples generation tool, instead opting for a simple dart and flutter sample that can be redirected to.

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 17, 2024 •
edited
Loading

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

Coverage ✔️
File Coverage

This check for test coverage is informational (issues shown here will not fail the PR).

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbols
License Headers ✔️ 
// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/samples/lib/brick_breaker.dart
pkgs/samples/lib/fibonacci.dart
pkgs/samples/lib/google_ai.dart
pkgs/samples/lib/hello_world.dart
pkgs/samples/lib/main.dart
pkgs/samples/lib/sunflower.dart

devoncarew
devoncarew approved these changes Nov 19, 2024
View reviewed changes
Copy link
Member

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

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

lgtm, but we should confirm w/ John how this'll interact with the new /:gistId route.

pkgs/dartpad_ui/firebase.json Outdated Show resolved Hide resolved
pkgs/dartpad_ui/lib/main.dart Outdated
@@ -59,10 +59,6 @@ class _DartPadAppState extends State<DartPadApp> {
path: '/',
builder: _homePageBuilder,
),
GoRoute(
Copy link
Member

Choose a reason for hiding this comment

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

@johnpryan should likely review this - he just landed a PR that introduces a :gistId route: #3096 (I believe this restores functionality from before the rewrite).

pkgs/dartpad_ui/lib/main.dart Show resolved Hide resolved
pkgs/samples/tool/samples.dart Show resolved Hide resolved
pkgs/samples/lib/default_dart.dart
@@ -1,5 +1,9 @@
// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
Copy link
Member

Choose a reason for hiding this comment

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

One drawback of having the copyright header here (and for default_flutter.dart) is that they'll show up for most dartpad snippets. I think it's ok to omit here, but to keep it for the more specific samples (sunflower, ...).

Copy link
Member Author

@parlough parlough Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In this PR, I ended up stripping the copyright from each snippet when loaded into DartPad, but keeping it in the source file. Is there any concern with that? As the license comments end up distracting from the samples anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I'd missed that! No real concerns.

@parlough parlough requested a review from johnpryan November 19, 2024 22:48
johnpryan
johnpryan approved these changes Nov 19, 2024
View reviewed changes
Copy link
Contributor

@johnpryan johnpryan left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for cleaning this up!

@parlough parlough merged commit 4d7507e into main Nov 20, 2024
12 checks passed
@parlough parlough deleted the misc/move-flutter-new-to-firebase-redirect branch November 20, 2024 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devoncarew devoncarew devoncarew approved these changes

@johnpryan johnpryan johnpryan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@parlough @johnpryan @devoncarew