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

Flavors documentation update #7867

Merged
merged 20 commits into from
Dec 22, 2022
Merged

Conversation

MiraMarshall
Copy link
Contributor

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

This PR updates the flavors documentation.
Issues fixed by this PR (if any): Fixes #ISSUE-NUMBER

Presubmit checklist

Copy link
Contributor

@domesticmouse domesticmouse 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 to me on the iOS side, but I'd like @jmagman's confirmation. I can't vouch for Android.


## Environment set up
Prerequisites:
* Xcode installed
Copy link
Contributor

Choose a reason for hiding this comment

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

Xcode is required for configuring Flavors for iOS.

Android Studio isn't strictly required for configuring Flavors for Android, but presumably is required for building an APK?

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.

Given that this doc has so many images, I'd like to see a staged version. Did you stage it? I should add that when we stage it, we generally put the staged URL in the PR, at the top.

src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
@sfshaza2 sfshaza2 added review.await-update Awaiting Updates after Edits and removed review.copy Awaiting Copy Review labels Nov 25, 2022
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

I think you're missing instructions related to the ios/Podfile when users have a plugin. Flutter plugins complicate this flow, it would be worth adding a plugin to the example to show what to do.

Particularly, ios/Podfile needs to be updated from

project 'Runner', {
  'Debug' => :debug,
  'Profile' => :release,
  'Release' => :release,
}

to

project 'Runner', {
  'Debug-development' => :debug,
  'Debug-production' => :debug,
  'Profile-development' => :release,
  'Profile-production' => :release,
  'Release-development' => :release,
  'Release-production' => :release,
}

Copy link
Contributor

@MaryaBelanger MaryaBelanger left a comment

Choose a reason for hiding this comment

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

This is so well done! Some formatting suggestions to make it easier to follow

src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
Copy link
Contributor

@MaryaBelanger MaryaBelanger left a comment

Choose a reason for hiding this comment

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

LGTM!

Left comments for some minor formatting inconsistencies you may or may not already be aware of, I think they're intentional anyway because formatting was rendering weirdly

src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
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.

Some of the feedback from my previous review hasn't been implemented. Let me know if you want to meet.

src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
@sfshaza2 sfshaza2 removed the review.copy Awaiting Copy Review label Dec 14, 2022
@sfshaza2
Copy link
Contributor

I would like to land this before 2023. I think, once the edits are incorporated, it's good to go. Open an issue if there are any other "TODOs" that we want to incorporate. thx!

src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
src/deployment/flavors.md Outdated Show resolved Hide resolved
@MiraMarshall MiraMarshall added review.copy Awaiting Copy Review and removed review.await-update Awaiting Updates after Edits labels Dec 22, 2022
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. @jmagman, if you still have issues with this page, either let @MiraMarshall know or file an issue. I believe she's addressed your issues.

Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

I think this would be a lot clearer if the screenshots showed a paid flavor next to free as we have in our sample app, since only having one flavor doesn't really make sense since in that case you could just update the default app without dealing with flavors.

However I know you want to get this merged ASAP. LGTM once my two last comments are addressed.

"request": "launch",
"type": "dart",
"program": "lib/main_development.dart",
"args": ["--flavor", "development", "--target", "lib/main_development.dart" ]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"args": ["--flavor", "development", "--target", "lib/main_development.dart" ]
"args": ["--flavor", "free", "--target", "lib/main_free.dart" ]

Comment on lines 120 to 121
Add the display name to **info.plist**. Create a new key in **info.plist** called **Bundle Display Name**
with the value `$(PRODUCT_NAME)`.
Copy link
Member

Choose a reason for hiding this comment

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

There should already be a Bundle Display Name key in that file (casing is Info.plist). How about:

Suggested change
Add the display name to **info.plist**. Create a new key in **info.plist** called **Bundle Display Name**
with the value `$(PRODUCT_NAME)`.
Add the display name to **Info.plist**. Update the **Bundle Display Name**
value to `$(PRODUCT_NAME)`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Jenn! I've made the changes and will address the screenshots with #7979

@jmagman jmagman mentioned this pull request Dec 22, 2022
11 tasks
@jmagman jmagman linked an issue Dec 22, 2022 that may be closed by this pull request
@MiraMarshall MiraMarshall merged commit 20dc771 into flutter:main Dec 22, 2022
@MiraMarshall MiraMarshall deleted the flavors-doc-update branch December 22, 2022 20:26
@sfshaza2 sfshaza2 removed the review.copy Awaiting Copy Review label Jul 10, 2023
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.

Add documentation for supporting flavors
5 participants