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

Update url-strategies.md #11221

Merged
merged 3 commits into from
Oct 16, 2024
Merged

Update url-strategies.md #11221

merged 3 commits into from
Oct 16, 2024

Conversation

chrismercredi
Copy link
Contributor

@chrismercredi chrismercredi commented Oct 3, 2024

You can't add flutter_web_plugins directly using pub add. Its only included as part of the project if required by other dependencies like go_router.

Description of what this PR is changing or adding, and why:

Added additional information on how to add flutter_web_plugins to project to use usePathUrlStrategy() function.

Issues fixed by this PR (if any):

You can't add flutter_web_plugins directly using pub add or other methods.

PRs or commits this PR depends on (if any):

Presubmit checklist

  • This PR is marked as draft with an explanation if not meant to land until a future stable release.
  • This PR doesn’t contain automatically generated corrections (Grammarly or similar).
  • This PR follows the Google Developer Documentation Style Guidelines — for example, it doesn’t use i.e. or e.g., and it avoids I and we (first person).
  • This PR uses semantic line breaks of 80 characters or fewer.

You can't add flutter_web_plugins directly using pub add. Its only included as part of the project if required by other dependencies like go_router.
@chrismercredi chrismercredi requested review from sfshaza2, parlough and a team as code owners October 3, 2024 01:21
Copy link

google-cla bot commented Oct 3, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

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.

lgtm

Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! It definitely makes sense to clarify that it doesn't need to be added from pub.

However, I believe relying on it as a transitive dependency isn't the only option. You can also specify it as a sdk dependency, similar to flutter_test.

dependencies:
  flutter:
    sdk: flutter
  flutter_web_plugins:
    sdk: flutter

What do you think about suggesting this instead? Since the depend_on_referenced_packages lint would report that it's not depended on if it it's retrieved through a transitive dependency.

Let me know what you think. Thanks again! :D

@sfshaza2
Copy link
Contributor

/gcbrun

@flutter-website-bot
Copy link
Collaborator

flutter-website-bot commented Oct 14, 2024

Visit the preview URL for this PR (updated for commit 62167ab):

https://flutter-docs-prod--pr11221-patch-1-t1uszx84.web.app

Updated based on feedback from parlough:

flutter#11221 (review)
@chrismercredi
Copy link
Contributor Author

Thanks for the PR! It definitely makes sense to clarify that it doesn't need to be added from pub.

However, I believe relying on it as a transitive dependency isn't the only option. You can also specify it as a sdk dependency, similar to flutter_test.

dependencies:
  flutter:
    sdk: flutter
  flutter_web_plugins:
    sdk: flutter

What do you think about suggesting this instead? Since the depend_on_referenced_packages lint would report that it's not depended on if it it's retrieved through a transitive dependency.

Let me know what you think. Thanks again! :D

Thanks for the feedback, I've updated it based on your suggestions :)

Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

Thanks so much for those adjustments! Clarifying this will definitely be super helpful :)

src/content/ui/navigation/url-strategies.md Outdated Show resolved Hide resolved
@parlough
Copy link
Member

/gcbrun

@parlough parlough merged commit 7ed8ad6 into flutter:main Oct 16, 2024
9 checks passed
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.

4 participants