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.

Sign up for GitHub

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

Jump to bottom

All steps are Completed #223

Draft
wants to merge 10 commits into
base: develop
Choose a base branch
from
Draft

All steps are Completed #223

wants to merge 10 commits into from

Conversation

shehzad-haroon
Copy link

@shehzad-haroon shehzad-haroon commented Nov 6, 2021 •
edited by gitpod-io bot
Loading

This change is Reviewable

hound[bot]
hound bot reviewed Nov 6, 2021
View reviewed changes
public/serviceworker.js Show resolved Hide resolved
public/serviceworker.js Outdated Show resolved Hide resolved
public/serviceworker.js Show resolved Hide resolved
sonatype-lift[bot]
sonatype-lift bot reviewed Nov 6, 2021
View reviewed changes
public/serviceworker.js Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Nov 6, 2021

This pull request introduces 1 alert when merging e29042b into 53f11f1 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@codecakes codecakes linked an issue Nov 6, 2021 that may be closed by this pull request
Convert reactjs website into an installable webapp with push notifications. #215
Open
@gitpod-io
Copy link

gitpod-io bot commented Nov 7, 2021

@lgtm-com
Copy link

lgtm-com bot commented Nov 7, 2021

This pull request introduces 1 alert when merging 5c1519d into 53f11f1 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

sonatype-lift[bot]
sonatype-lift bot reviewed Nov 7, 2021
View reviewed changes
public/serviceworker.js Outdated Show resolved Hide resolved
codecakes
codecakes reviewed Nov 7, 2021
View reviewed changes
Copy link
Contributor

@codecakes codecakes left a comment

Choose a reason for hiding this comment

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

these are drop-in replacements. to be done later since we need to provide our own logo with these requirements.

codecakes
codecakes requested changes Nov 7, 2021
View reviewed changes
Copy link
Contributor

@codecakes codecakes left a comment

Choose a reason for hiding this comment

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

ive left a few suggestions

public/index.html Outdated Show resolved Hide resolved
public/index.html Outdated Show resolved Hide resolved
public/index.html Outdated
if ("serviceWorker" in navigator) {
window.addEventListener("load", function () {
navigator.serviceWorker.register("/serviceworker.js").then(
function (registration) {
Copy link
Contributor

Choose a reason for hiding this comment

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

enable this only if the DEBUG variable is set to true. you can import debug variable from constants.js;
If there isn't one, then create a variable as such. follow the pattern there.

public/index.html Outdated Show resolved Hide resolved
public/index.html Outdated
div.style.display = "none";
// Clear the deferredPrompt so it can be garbage collected
deferredPrompt = null;
// Optionally, send analytics event to indicate successful install
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to trigger alert. although this would be a nice to have event trigger, you can add a task here like:
TODO: send analytics event to indicate successful pwa app install

public/serviceworker.js Show resolved Hide resolved
public/serviceworker.js Outdated Show resolved Hide resolved
public/serviceworker.js
@@ -0,0 +1,39 @@
/* eslint-disable array-callback-return */
Copy link
Contributor

Choose a reason for hiding this comment

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

can you wrap this inside a class and export?

public/serviceworker.js
event.waitUntil(
caches.open(CACHE_NAME)
.then((cache) => {
console.log('Opened cache');
Copy link
Contributor

Choose a reason for hiding this comment

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

if you are debugging using these logs, print them only if DEBUG is enabled. see my comment on DEBUG somewhere above in the review

public/serviceworker.js Outdated Show resolved Hide resolved
@codecakes
Copy link
Contributor

@shehzad-haroon may want to fix the file conflict on public/index.html

@shehzad-haroon
Copy link
Author

branch this out in a separate scss/css and import it here(Solved)
can you separate this out inside the catch call. that is more explicit(Solved)
refactor into a separate css/scss and then import here(Solved)
change to SELF(Solved)
why not this?(Solved)
declare this. use 'esversion: 6'(Solved)
enable this only if the DEBUG variable is set to true. you can import debug variable from constants.js;
If there isn't one, then create a variable as such. follow the pattern there.(Pending)
can you wrap this inside a class and export?(Pending)
no need to trigger alert. although this would be a nice to have event trigger, you can add a task here like:
TODO: send analytics event to indicate successful pwa app install(Pending)

@codecakes
Copy link
Contributor

@shehzad-haroon
when i click on the installed app from my chrome apps, the app window does not load anything.

branch this out in a separate scss/css and import it here(Solved) can you separate this out inside the catch call. that is more explicit(Solved) refactor into a separate css/scss and then import here(Solved) change to SELF(Solved) why not this?(Solved) declare this. use 'esversion: 6'(Solved) enable this only if the DEBUG variable is set to true. you can import debug variable from constants.js; If there isn't one, then create a variable as such. follow the pattern there.(Pending) can you wrap this inside a class and export?(Pending) no need to trigger alert. although this would be a nice to have event trigger, you can add a task here like: TODO: send analytics event to indicate successful pwa app install(Pending)

@codecakes codecakes marked this pull request as draft November 30, 2021 13:30
@codecakes codecakes added the good first issue Good for newcomers label Nov 30, 2021
@codecakes codecakes added help wanted Extra attention is needed up-for-grabs labels Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codecakes codecakes codecakes requested changes

@hound hound[bot] hound[bot] left review comments

@sonatype-lift sonatype-lift[bot] sonatype-lift[bot] left review comments

@PunitSoniME PunitSoniME Awaiting requested review from PunitSoniME

Requested changes must be addressed to merge this pull request.

Assignees

@shehzad-haroon shehzad-haroon

Labels
enhancement New feature or request good first issue Good for newcomers Hacktoberfest hacktoberfest-accepted help wanted Extra attention is needed needs-review Review this PR up-for-grabs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Convert reactjs website into an installable webapp with push notifications.
3 participants
@shehzad-haroon @codecakes @farhanzaki-omegasoft