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

Clarify initial route usage with cached engines. #3551

Conversation

matthew-carroll
Copy link
Contributor

Clarify initial route usage with cached engines.

@googlebot googlebot added the cla: yes Contributor has signed the Contributor License Agreement label Jan 16, 2020
Copy link
Contributor

@sfshaza2 sfshaza2 left a comment

Choose a reason for hiding this comment

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

This LGTM, though I am wondering why you've added what appears to be identical text/code to two different files. Are there differences I am missing with my visual scan? I know one file addresses Flutter fragments and the other addresses Flutter screens, but the sections seem to be the same.

Otherwise, LGTM

@matthew-carroll
Copy link
Contributor Author

@sfshaza2 the question of using an initial route with a cached engine applies to both FlutterActivity and FlutterFragment in identical ways, so it's identical text. I could have used just the term Activity in the 1st one and just Fragment in the 2nd one, but since everything else is identical, I figured best to make them the same so any future updates can be copied and pasted with no additional work.

Also, it looks like we now have some kind of enforced review process. Can you explicitly add your approval via the GitHub button so that I'll be allowed to merge?

@matthew-carroll
Copy link
Contributor Author

@RedBrogdon I added you as a reviewer to this in case you have a moment to press the GitHub approve button. I'm currently blocked from merging until someone does that...

@legalcodes legalcodes self-requested a review January 17, 2020 23:42
@legalcodes
Copy link
Contributor

@matthew-carroll feel free to reach out to me as well if the situation arises again, happy to help out.

matthew-carroll and others added 2 commits January 17, 2020 15:52
Co-Authored-By: Jon Tippens <legalcodes@users.noreply.github.com>
Co-Authored-By: Jon Tippens <legalcodes@users.noreply.github.com>
Changing the initial route property of the navigation channel after the initial
execution of `runApp()` has no effect. Developers who would like to use
the same `FlutterEngine` between different `Activity`s and `Fragment`s and
switch the route between those displays need to setup a method channel and
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
switch the route between those displays need to setup a method channel and
switch the route between those displays need to set up a method channel and

@matthew-carroll
Copy link
Contributor Author

Thanks @legalcodes. I'm gonna add you to another website PR.

@legalcodes
Copy link
Contributor

Sure, np

@legalcodes
Copy link
Contributor

Also, if this has urgency no need to worry about the nits. We can track them and make the updates at a later time.

@sfshaza2
Copy link
Contributor

@matthew-carroll, @legalcodes, I wouldn't have approved this PR. Rather than adding identical text to two pages, I would have refactored it by creating a _foo.md file and including it into both places. Otherwise it's difficult to maintain. You can see this sort of thing done in the /src/docs/get-started/install directory.

@legalcodes
Copy link
Contributor

@sfshaza2 Addressed in #3576

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Contributor has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants