Skip to content
This repository has been archived by the owner on May 27, 2021. It is now read-only.

Fix #7: Generate manifest.json in output root #20

Merged
merged 4 commits into from
Oct 26, 2018

Conversation

DevLab2425
Copy link
Contributor

Related Issue

Resolves #7

Summary of Changes

I noticed the Webpack Favicon plugin was generating a manifest.json, but in a subfolder (/icons). By modifying where the icons are generated, the manifest.json is generated in the root of the output folder, which is typically a best practice when including a manifest.

With that, a hardcoded <link /> was added to the HTML to include the manifest.json file.

This PR may not complete the PWA task since it does not include a service worker, however, it's a step in the PWA direction.

@thescientist13 thescientist13 added the enhancement New feature or request label Oct 24, 2018
Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

So one thing about this is that without the service worker, this probably wouldn't be "true" PWA, right? Just curious if you would consider this one step in implementing a PWA? Should define more incremental steps in the ticket to track as a master list of features to add?

For instance, if a service worker is added, it would require the site has to run over HTTPS. Is that something we would want to make default in this app? Or just provide instructions for how to add? Also, managing the cache of service works can be tricky, so that seems to make more of a case to account for documentation / contingencies / opt-in ability.

Thoughts?

@DevLab2425
Copy link
Contributor Author

You're right that a SW is required for the 💯 in Lighthouse, but manifest.json gets the benefit of clients using the icons and metadata where possible. The SW does need HTTPS, but it seems the site defaults to that already, no? And with the manifest.json, the SW would be the last piece if/when it was prioritized. There's a nice Webpack plugin that includes the SW file into the build and caches the assets when the app loads.

As far as whether it should be included in CEA, maybe a CLI flag? Could put the pieces in place for something basic, with a set of dedicated instructions on how and where to make changes?

Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Cool, so sounds like we would technically want to keep #8 open and add these takeaways / additional actions items and keep it open after merging this. I'll make the updates in the issue.

but it seems the site defaults to that already, no?

not sure what you mean here?

@DevLab2425
Copy link
Contributor Author

DevLab2425 commented Oct 26, 2018

EDIT:
Sorry, totally side stepped the first part.

This PR may not "resolve" #7 completely, so could maybe remove that from the description. Maybe, instead just call this new enhancement in the right direction? #8 would be additional to #7.

but it seems the site defaults to that HTTPS already, no?

@thescientist13
Copy link
Member

@DevLab2425
a good point about #7 and #8. This seems unobtrusive enough so let's get it in and do the rest of the work in #8.

but it seems the site defaults to that HTTPS already, no?

hah, yes, I knew you mean HTTPS, I was more inquiring about your assumption of default behavior. To my knowledge, this project doesn't yet make any assumptions about that, but by enabling a SW by default, it would.

@thescientist13 thescientist13 merged commit 91d6802 into ProjectEvergreen:master Oct 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

As a developer I would like support for making a Progressive (Evergreen) Web Application
2 participants