-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[video_player] Add optional web options [web] #4551
[video_player] Add optional web options [web] #4551
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
@ditman @stuartmorgan Web PR for #3259. |
c89c8e7
to
2175f44
Compare
2175f44
to
a2b2329
Compare
packages/video_player/video_player_web/lib/src/video_player.dart
Outdated
Show resolved
Hide resolved
a2b2329
to
bfc2d6d
Compare
bfc2d6d
to
d740422
Compare
@@ -1,6 +1,7 @@ | |||
## NEXT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## NEXT | |
## 2.1.0 |
This needs to be published to be usable, right? If so, we want to bump the version to 2.1.0
, here and in the pubspec.yaml
of the package!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Done)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see the history since this PR has been force-pushed, but it looks like you undid an existing change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stuartmorgan Apologies, added back 688077f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual package version bump is still reverted, as indicated by CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, I think this is ready to land. Second opinion, @stuartmorgan?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with two nits.
packages/video_player/video_player_web/lib/src/video_player.dart
Outdated
Show resolved
Hide resolved
47503fb
to
dff9524
Compare
@stuartmorgan @ditman Requested changes pushed :) |
@@ -1,6 +1,7 @@ | |||
## NEXT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see the history since this PR has been force-pushed, but it looks like you undid an existing change.
688077f
to
150bc52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating review state per my comment above; this is failing CI because it doesn't actually contain a version bump.
150bc52
to
9a1e7ff
Compare
@ditman @stuartmorgan Please re-reviewed and let me know if anything needs to be changed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the PR to bump the version, since that was still missing.
LGTM with that change.
Sorry, looks like I just missed adding the label to this. |
flutter/packages@22d4754...aaae5ef 2023-09-08 43054281+camsim99@users.noreply.github.com [tool] Add Android dependency (gradle) option to update dependencies command (flutter/packages#4757) 2023-09-08 43054281+camsim99@users.noreply.github.com [camerax] Implement resolution configuration (flutter/packages#3799) 2023-09-07 engine-flutter-autoroll@skia.org Manual roll Flutter from 685ce14 to aea4552 (64 revisions) (flutter/packages#4870) 2023-09-07 ditman@gmail.com [rfw, ci] Regenerate goldens, manually roll #4835 (flutter/packages#4862) 2023-09-07 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 3.6.0 to 4.0.0 (flutter/packages#4845) 2023-09-07 defuncart@gmail.com [video_player] Add optional web options [web] (flutter/packages#4551) 2023-09-07 me@nils.re [flutter_markdown] Remove `ignore: avoid_init_to_null` since the package uses Dart 3 (flutter/packages#4852) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Web PR for Video Player Web Options (#3259).
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.