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.

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

Make BeamPage.key required #443

Merged
merged 1 commit into from
Jun 17, 2022

Conversation

Goddchen
Copy link
Contributor

@Goddchen Goddchen commented Jan 3, 2022

Force BeamPage.key to be set in order for Navigator to be able to tell BeamPages apart.

@Goddchen
Copy link
Contributor Author

Goddchen commented Jan 3, 2022

@slovnicki Is it okay like this or should it be required LocalKey? key so that devs can set the key to null if they wish to? 🤔

@codecov
Copy link

codecov bot commented Jan 3, 2022

Codecov Report

Merging #443 (a3d52e3) into master (a6bb5f3) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #443   +/-   ##
=======================================
  Coverage   96.21%   96.21%           
=======================================
  Files          13       13           
  Lines         739      739           
=======================================
  Hits          711      711           
  Misses         28       28           
Impacted Files Coverage Δ
package/lib/src/beam_page.dart 97.33% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6bb5f3...a3d52e3. Read the comment docs.

@slovnicki
Copy link
Owner

I now realize it must be pretty awkward for people to see this PR standing without attention like this.
This is because we talked about it privately, but yeah... Would be best I leave some comments here too.

Short story: This PR is waiting v2 because it will be a breaking change.

@slovnicki slovnicki added this to the v2 milestone Jun 17, 2022
@slovnicki slovnicki merged commit 4f66118 into slovnicki:master Jun 17, 2022
@slovnicki
Copy link
Owner

Well, @Goddchen, the time has come to merge this 😄

@Goddchen
Copy link
Contributor Author

Omg, 2.0.0 incoming 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants