Skip to content

Conversation

@sreevani-ship-it
Copy link
Contributor

@sreevani-ship-it sreevani-ship-it commented Apr 30, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[x] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Currently, the doc does not have content that explains updates to the index.html with the ng add @angular/pwa command.
Issue Number: 23373 #23373

What is the new behavior?

Added doc to explain the updates to index.html file with the ng add @angular/pwa command

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@mary-poppins
Copy link

You can preview 0660306 at https://pr23616-0660306.ngbuilds.io/.

@sreevani-ship-it sreevani-ship-it requested a review from Brocco April 30, 2018 17:45
Copy link
Contributor

@Brocco Brocco 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
Contributor

Choose a reason for hiding this comment

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

There's a typo here, it's gotta be Progressive

@mary-poppins
Copy link

You can preview e36d8eb at https://pr23616-e36d8eb.ngbuilds.io/.

@mary-poppins
Copy link

You can preview a96507d at https://pr23616-a96507d.ngbuilds.io/.

Copy link
Contributor

@jenniferfell jenniferfell left a comment

Choose a reason for hiding this comment

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

Please clarify which "installed PWA" because we haven't used that phrase in these docs before. Other comments are optional, but while you're here... :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

@Brocco: Double-checking...we don't need to say that is adds meta tags for Description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked with him and he confirmed that the Description meta tag is not added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unclear reference: We've never mentioned "the installed Progressive Web App (PWA)" using that phrase before. Is this the app/project to which we're currently adding service worker support?

And I don't think we've been clear in the past about what icon file we're adding any why. What's the icon file for? Also....as a developer, I might wonder why you're adding icon files to my app? Is this the file we're talking about: favicon.ico?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that we are referring to the project to which we are adding service worker. And icon files are the app's icons. @Brocco : please confirm

Copy link
Contributor

Choose a reason for hiding this comment

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

Missing comma: Creates the service worker configuration file called ngsw-config.json, which specifies the caching behaviors and other settings.

Also...maybe add a link from this filename to the doc file about config options?

@jenniferfell jenniferfell added comp: docs target: major This PR is targeted for the next major release labels Apr 30, 2018
@jenniferfell jenniferfell added this to the v6-candidates milestone Apr 30, 2018
@mary-poppins
Copy link

You can preview 759bd8f at https://pr23616-759bd8f.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 606db2b at https://pr23616-606db2b.ngbuilds.io/.

@jenniferfell
Copy link
Contributor

I still think we can be more clear about this item:
"Installs icon files to support the installed Progressive Web App (PWA)."

But we can come back as necessary if others are confused. Nothing technically incorrect or missing.

@jenniferfell
Copy link
Contributor

@IgorMinar Hi. The essentials have been updated. With a pull-approve member approval, I think we can merge.

@IgorMinar IgorMinar added the action: merge The PR is ready for merge by the caretaker label May 2, 2018
@IgorMinar IgorMinar changed the title docs: add doc to include updates to the index.html with the new ng add command docs(service-worker): add doc to include updates to the index.html with the new ng add command May 2, 2018
@IgorMinar IgorMinar closed this in bfad6b4 May 2, 2018
IgorMinar pushed a commit that referenced this pull request May 3, 2018
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants