Skip to content

Conversation

@xfqwdsj
Copy link
Contributor

@xfqwdsj xfqwdsj commented Mar 25, 2023

New Pull Request Checklist

Issue Description

Closes: #861

Approach

Remove the specified vsync property.

TODOs before merging

  • Add tests
  • Add changes to documentation (guides, repository pages, in-code descriptions)
  • A changelog entry

@parse-github-assistant
Copy link

parse-github-assistant bot commented Mar 25, 2023

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

Copy link
Member

@Nidal-Bakir Nidal-Bakir left a comment

Choose a reason for hiding this comment

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

Could you please add a changelog entry and bump the version?

@xfqwdsj
Copy link
Contributor Author

xfqwdsj commented Mar 25, 2023

Could you please add a changelog entry and bump the version?

OK I am trying. Thank you!

Copy link
Member

@Nidal-Bakir Nidal-Bakir left a comment

Choose a reason for hiding this comment

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

You also can remove this line.

with SingleTickerProviderStateMixin

@xfqwdsj
Copy link
Contributor Author

xfqwdsj commented Mar 25, 2023

You also can remove this line.

with SingleTickerProviderStateMixin

finished! Please check it out, thank you.

@xfqwdsj xfqwdsj changed the title fix: No named parameter with the name vsync. fix: Remove deprecated parameter vsync form AnimatedSize. Mar 25, 2023
Nidal-Bakir
Nidal-Bakir previously approved these changes Mar 25, 2023
Copy link
Member

@Nidal-Bakir Nidal-Bakir left a comment

Choose a reason for hiding this comment

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

Thanks, Looks good.

@mtrezza
Copy link
Member

mtrezza commented Mar 25, 2023

If we remove a property and therefore remove support for currently supported Flutter SDK versions then this is a breaking change. Let's discuss this in the issue.

mbfakourii
mbfakourii previously approved these changes Mar 26, 2023
@mbfakourii
Copy link
Member

If we remove a property and therefore remove support for currently supported Flutter SDK versions then this is a breaking change. Let's discuss this in the issue.

I don't think it is a breaking change. Because it is a deprecated variable. Also, this issue and change is inside Parse and does not affect the api

@mtrezza
Copy link
Member

mtrezza commented May 7, 2023

@mbfakourii Could you comment in the issue (see link in my previous comment)? From the discussion there it seems that this is in fact a breaking change.

@mbfakourii
Copy link
Member

@mbfakourii Could you comment in the issue (see link in my previous comment)? From the discussion there it seems that this is in fact a breaking change.

I checked, is it true that I think it is a breaking change, with these qualities, we have a lot of breaking change in PRs!?

@Nidal-Bakir
Copy link
Member

@mtrezza
we can start with this PR

@mtrezza mtrezza added the state:breaking Breaking change requires major version increment and `BREAKING CHANGE` commit message label May 13, 2023
@Nidal-Bakir
Copy link
Member

No, its not

@mtrezza mtrezza dismissed stale reviews from mbfakourii and Nidal-Bakir via 8f49447 May 13, 2023 22:35
@mtrezza
Copy link
Member

mtrezza commented May 13, 2023

Sorry, I forgot that there was discussion here already about this. So since removing a deprecated param is breaking, I've added an additional entry in the BREAKING CHANGES section.

I think one of the problems we have now is that the CI for Flutter generally fails. So maybe before we merge this PR we can fix the general issue so that the CI runs properly before we merge any more PRs?


Edit: the CI works again after resolving the conflicts, so I guess we can merge this.

@codecov
Copy link

codecov bot commented May 13, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (7faf800) 38.69% compared to head (8f49447) 38.69%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #864   +/-   ##
=======================================
  Coverage   38.69%   38.69%           
=======================================
  Files          56       56           
  Lines        3277     3277           
=======================================
  Hits         1268     1268           
  Misses       2009     2009           
Impacted Files Coverage Δ
...ackages/flutter/lib/src/utils/parse_live_list.dart 0.00% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Nidal-Bakir
Copy link
Member

The issue with CI is related to this PR and issue #877. Once we merge/fix them, the CI tests for Flutter should pass.

@mtrezza mtrezza changed the title fix: Remove deprecated parameter vsync form AnimatedSize. fix: Remove deprecated parameter vsync from AnimatedSize May 13, 2023
@mtrezza mtrezza merged commit 518f768 into parse-community:master May 13, 2023
@xfqwdsj
Copy link
Contributor Author

xfqwdsj commented May 14, 2023

This is great! Thanks for your work!

@mbfakourii mbfakourii mentioned this pull request Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state:breaking Breaking change requires major version increment and `BREAKING CHANGE` commit message

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No named parameter with the name 'vsync'.

4 participants