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 uri construction for queryParameters #563

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Lorenzohidalgo
Copy link
Contributor

This PR intends to help avoid replicating code when using the beamer package and URI's with query parameters. While refactoring my code I came across multiple navigation calls where I could avoid manually constructing the URI if the beamer package accepted query parameters as an input.

I've made the following changes:

  • added Map<String, dynamic>? queryParameters as an optional input parameter for: beamToNamed, beamToReplacementNamed and popToNamed
  • added the private function String constructUri(String uri, Map<String, dynamic>? queryParameters) that return the URI with the formatted query parameters appended at the end.
  • added unit tests for the new function.

@slovnicki feel free to request any additional changes to my code or discard the pull request if you don't consider the changes relevant/useful.

@Lorenzohidalgo Lorenzohidalgo changed the title Added autmatic uri construction for queryParameters Added uri construction for queryParameters Aug 27, 2022
@codecov
Copy link

codecov bot commented Aug 27, 2022

Codecov Report

Merging #563 (738b002) into master (1aa33d3) will increase coverage by 0.29%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #563      +/-   ##
==========================================
+ Coverage   96.62%   96.91%   +0.29%     
==========================================
  Files          13       13              
  Lines         888      909      +21     
==========================================
+ Hits          858      881      +23     
+ Misses         30       28       -2     
Impacted Files Coverage Δ
package/lib/src/beam_state.dart 100.00% <100.00%> (ø)
package/lib/src/beamer.dart 96.00% <100.00%> (+4.51%) ⬆️
package/lib/src/beamer_delegate.dart 95.12% <100.00%> (+0.48%) ⬆️
package/lib/src/utils.dart 100.00% <100.00%> (ø)
package/lib/src/beam_guard.dart 93.33% <0.00%> (-1.79%) ⬇️
package/lib/src/beam_page.dart 98.09% <0.00%> (-0.02%) ⬇️
package/lib/src/beamer_back_button_dispatcher.dart 95.45% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@slovnicki
Copy link
Owner

@Lorenzohidalgo Thanks for the PR! 💙

This is definitely a useful feature, but now that I think of it... Maybe we should switch to Uri altogether instead of String for named beaming 🤔 Sure, this will be a breaking change, but this is expected for the v2 we're working on. Let me know what you think about that. The only thing that concerns me is that it is a significant breaking change and a bit of a hassle for apps that really use only path. I wish we could take the best of both worlds without over-complicating the API.

We should also update all the functions in BeamerDelegate to make use of this new feature, be it your constructUri helper or complete switch to Uri. And if we stay with the constructUri option, maybe a better place for it would be utils.dart.

@Lorenzohidalgo
Copy link
Contributor Author

Hi @slovnicki, Thanks for your feedback! I also agree that maybe switching to Uri altogether would resolve the issue, but as you mentioned it would be a breaking change and more complex/cumbersome for users with more straightforward implementations. To avoid creating a breaking change or making it more complicated for other users I went with the constructUri approach.

Either way, I would be happy to help with implementing this further. Just let me know If I should make the complete change to Uri or just continue updating the functions to use constructUri as mentioned in your last paragraph.

@slovnicki
Copy link
Owner

@Lorenzohidalgo Yeah, it might be best if we don't do the full transition to Uri yet. Let's complete your constructUri proposal and maybe we can discuss the transition to Uri after, in some other PR.

@Lorenzohidalgo
Copy link
Contributor Author

Perfect @slovnicki , I'll implement the requested changes and let you know once it's ready for the next review 😉

@Lorenzohidalgo
Copy link
Contributor Author

Hey there @slovnicki ,

I've finally had a moment to fix the requested changes:

We should also update all the functions in BeamerDelegate to make use of this new feature, be it your constructUri helper or complete switch to Uri. And if we stay with the constructUri option, maybe a better place for it would be utils.dart.

Let me know if anything else should be updated 😉

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