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

Added the ability to pass and receive arguments #97

Closed
wants to merge 12 commits into from

Conversation

ianko
Copy link

@ianko ianko commented Mar 29, 2019

Solves #46, #78 and #93.

@codecov-io
Copy link

codecov-io commented Mar 29, 2019

Codecov Report

Merging #97 into master will decrease coverage by 1.47%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
- Coverage   50.99%   49.51%   -1.48%     
==========================================
  Files           3        3              
  Lines         202      208       +6     
==========================================
  Hits          103      103              
- Misses         99      105       +6     
Impacted Files Coverage Δ
lib/src/common.dart 77.27% <ø> (ø)
lib/src/router.dart 5.81% <0.00%> (-0.44%) ⬇️
lib/src/tree.dart 81.00% <ø> (ø)

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 3d5a90d...b4d08de. Read the comment docs.

@bluemix
Copy link

bluemix commented Apr 3, 2019

I hope to merge this feature

@lukef
Copy link
Collaborator

lukef commented Apr 3, 2019

I will be reviewing it but I'm not sure this will make a release before 2.0 (where the feature is already built using a different approach). This is a breaking change and I'm not really willing to roll a 2.0 just for this.

@bluemix
Copy link

bluemix commented Apr 3, 2019

Ok, thanks for your reply.
is there any date when 2.0 will be released?

@Kunoacc
Copy link

Kunoacc commented Apr 21, 2019

Ok, thanks for your reply.
is there any date when 2.0 will be released?

+1

@rchavik
Copy link

rchavik commented Sep 18, 2019

I will be reviewing it but I'm not sure this will make a release before 2.0 (where the feature is already built using a different approach). This is a breaking change and I'm not really willing to roll a 2.0 just for this.

Is there a branch for 2.0? I can only find routable which is for something else afaik

@WingCH
Copy link

WingCH commented May 7, 2020

I hope to merge this feature

@softmarshmallow
Copy link

this seems quite critical, https://github.com/Flutterando/modular supports arguments pass.

ianko added 2 commits June 5, 2020 13:30
…a-master

# Conflicts:
#	CHANGELOG.md
#	example/lib/components/demo/demo_simple_component.dart
#	example/lib/components/home/home_component.dart
#	example/lib/config/route_handlers.dart
#	example/lib/config/routes.dart
#	lib/src/router.dart
#	pubspec.yaml
@codecov-commenter
Copy link

codecov-commenter commented Jun 5, 2020

Codecov Report

Merging #97 into master will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #97   +/-   ##
=======================================
  Coverage   49.51%   49.51%           
=======================================
  Files           3        3           
  Lines         208      208           
=======================================
  Hits          103      103           
  Misses        105      105           
Impacted Files Coverage Δ
lib/src/common.dart 77.27% <ø> (ø)
lib/src/router.dart 5.81% <0.00%> (ø)
lib/src/tree.dart 81.00% <ø> (ø)

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 30ae437...49cd013. Read the comment docs.

@lukepighetti
Copy link
Owner

Hi! Thanks for the contribution. I took your idea and added custom RouteSettings to the latest release, so I'm closing this. Cheers!

@softmarshmallow
Copy link

@lukepighetti Hm... you should have take more concern here. @ianko poured some hard work here. You could have simply just care more. It's been around more than a year

@lukepighetti
Copy link
Owner

lukepighetti commented Oct 17, 2020

@lukepighetti Hm... you should have take more concern here. @ianko poured some hard work here. You could have simply just care more. It's been around more than a year

Unfortunately I triaged the issues before the PRs, and in doing so I had already implemented RouteSettings based arguments by the time I triaged this. There is no doubt that a lot of care was put in this PR but it just didn't work out. Thanks for the thoughtful message.

Since there is now considerable overlap in features between this PR and master, and we're only dealing with low hanging fruit so we can move on to implementing Fluro in Pages / Navigator 2.0, there is nothing actionable so I won't be reopening this PR.

@ianko
Copy link
Author

ianko commented Oct 18, 2020

@softmarshmallow thanks for the consideration.

I am fine with the solution. I am glad the feature is now available, it doesn't matter in which way.

I am even happier to see this project moving once again and with plans for the 2.0.

You can count on me @lukepighetti.

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.

None yet

10 participants