packages icon indicating copy to clipboard operation
packages copied to clipboard

[video_player]add minHeight property of VideoProgressIndicator

Open ManInTheWind opened this issue 2 years ago • 2 comments

add nullable minHeight of VideoProgressIndicator,which is not destructive and quiet convenient to set process indicator height

image

List which issues are fixed by this PR. You must list at least one issue.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

  • [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
  • [x] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • [x] I signed the CLA.
  • [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • [x] I listed at least one issue that this PR fixes in the description above.
  • [x] I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • [x] I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • [x] I updated/added relevant documentation (doc comments with ///).
  • [x] I added new tests to check the change I am making, or this PR is test-exempt.
  • [x] All existing and new tests are passing.

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

ManInTheWind avatar Mar 05 '23 10:03 ManInTheWind

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Mar 05 '23 10:03 google-cla[bot]

This will help https://www.youtube.com/watch?v=IMQdSTlTXjA

BraveEvidence avatar Mar 15 '23 11:03 BraveEvidence

Thanks for the contribution! It's not clear to me from the description and screenshot what the actual problem you are trying to solve is. Please file an issue (as requested in the PR template), and explain in the issue what the use case is and the problem you are solving, so that we have context to review this.

You’ve checked boxes in the PR checklist above that are not reflected in this PR, so I’m assuming this is a work in progress and am marking it as a Draft. Please review the checklist, updating the PR as appropriate, and when the checklist reflects the state of the PR as posted please feel free to mark it as ready for review.

stuartmorgan-g avatar Apr 04 '23 19:04 stuartmorgan-g

@stuartmorgan Hi!Thank you for your kindly reply.The problem(maybe its not a really problem), as we all know, the VideoProcessIndicator is build with LinearProgressIndicator, and sure we can set or change the indicator height through ProgressIndicatorThemeData, however I think set the minHeigth by pass the minHeight paramater to VideoProcessIndicator will be more easy for people to dynamic change it. For example , when people drag the area of proccess indicator to forward or backward , we can just setState the heigth variate to brace it and see more clearly, after drag gesture we set the normal Height back.So I think expose the minHeigth paramater will make us to achieve this effect more easier.

I'm sorry that I actually didnt updated pubspec.yaml and CHANGELOG.md. I only change the code and add test case. Please Let me know how to follow up on the next move about this PR.

Thank you!

ManInTheWind avatar Apr 05 '23 04:04 ManInTheWind

Please Let me know how to follow up on the next move about this PR.

My previous comment explains what the next steps are.

stuartmorgan-g avatar Apr 05 '23 10:04 stuartmorgan-g

@ManInTheWind Per my comment above:

Please file an issue (as requested in the PR template), and explain in the issue what the use case is and the problem you are solving, so that we have context to review this.

There's still not a link to an issue here; see also the 2nd item here about why this is important.

Also, on the version change, you need to do those steps, not just uncheck the boxes.

stuartmorgan-g avatar Apr 11 '23 19:04 stuartmorgan-g

Since this is marked as a draft and hasn't been updated in the last month I'm going to close it to clean out our review queue. Please don't hesitate to submit a new PR if you decide to revisit this. Thanks!

stuartmorgan-g avatar May 09 '23 19:05 stuartmorgan-g