-
-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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
Toast for Service Worker #2426
Toast for Service Worker #2426
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting this started. I'll defer to @gaearon as he had some opinions about the effectiveness of toasts from the other thread we're discussing solutions on.
} | ||
|
||
Toast.defaultProps = { | ||
timeout: 3000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A 3s timeout is consistent with some of the other implementations that have opted for this pattern in case anyone is wondering: https://github.com/GoogleChrome/voice-memos/blob/bcb225a41d3dfcec5e9805adf33d27995840c9f0/src/scripts/libs/Toaster.js#L41.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also seen "dismiss" button some implementation, but I think we can leave it to the users? edit: nvm, updated it with dismiss button
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think you made the right call with the latest set of updates around dismissal.
@@ -39,6 +60,8 @@ export default function register() { | |||
console.error('Error during service worker registration:', error); | |||
}); | |||
}); | |||
} else { | |||
renderToast('Development mode started.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the goal of this to ease users into the idea of the toast UI now being available? I haven't seen other implementations of the toast showing a dev mode message but perhaps it's another awareness moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so they can see how it look if they want to style it differently :)
} else { | ||
// At this point, everything has been precached. | ||
// It's the perfect time to display a | ||
// "Content is cached for offline use." message. | ||
console.log('Content is cached for offline use.'); | ||
showMessage('Content is cached for offline use.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One possible addition would be to also include a "learn more" link that pointed to the documentation for additional context. Using a timeout of longer than 3 seconds might be appropriate in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this code is running on production. I don't think it's a good default for the user to have a link to create-react-app in their production code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeffposnick I added readme link in dev mode but I think it would be made more sense to link it to the full readme.md instead of the specific section of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Service Workers are not enabled in development mode, maybe we should let the developer know why the toast exists.
We could also add some information in registerServiceWorker.js
about how to remove service workers.
"In production your users will see this toast to notify them service workers are enabled. See registerServiceWorker.js for more info"
What about instead of using the word 'content' we say 'app'.
A new version of this app is available. Please refresh your browser.
This app had been made available for offline use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it a nice enhancement if you make the word refresh
as a link on click of which a refresh is triggered?
bf1eb23
to
654e47c
Compare
@@ -0,0 +1,39 @@ | |||
.Toast { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth name spacing this, if there is any expectation the toast will appear in production?
.cra-Toast
maybe ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to use this as a template for the user to customize it. So I think it's okay to leave it not namespaced. CMIIW
I miss an option to switch notification and bundled stylesheets off by config option. |
} | ||
|
||
.Toast a, | ||
.Toast-button { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this behave more like a link?
cursor: pointer
Toast-button:hover { text-decoration: underline }
And maybe remove the uppercasing from all links, and change it to just so DISMISS is in uppercase?
line-height: 24px; | ||
border-top-left-radius: 2px; | ||
border-top-right-radius: 2px; | ||
display: flex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a z-index
just to ensure this is on top?
const domId = 'toast'; | ||
let dom = document.getElementById(domId); | ||
|
||
if (!dom) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is suppose to be once off on load. Maybe we should remove the created dom node after the timeout ends / it's dismissed.
Don't think its necessary but could be nice to avoid any possibility of conflicting styles / layout.
This looks great @viankakrisna I've tested a bit across browsers + sizes (no IE though) and worked fine. I have made a couple optional suggestions. I'd like to see this merged in asap. |
@ro-savage thanks for the feedback! I'll hold off making changes until we are sure where we are going with service worker. On a second thought, maybe inline style is the way to go for this toast for the issue with the namespace. If SW ends up being an optional feature, I don't think we need the Toast component to be in a separate file (too much noise for the user). |
Hello! I'm a bot that helps facilitate testing pull requests. Your pull request (commit 873a617) has been released on npm for testing purposes. npm i react-scripts-dangerous@1.0.15-873a617.0
# or
yarn add react-scripts-dangerous@1.0.15-873a617.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.15-873a617.0 folder/ Note that the package has not been reviewed or vetted by the maintainers. Only install it at your own risk! Thanks for your contribution! |
@@ -56,12 +95,12 @@ function registerValidSW(swUrl) { | |||
// the fresh content will have been added to the cache. | |||
// It's the perfect time to display a "New content is | |||
// available; please refresh." message in your web app. | |||
console.log('New content is available; please refresh.'); | |||
showMessage('New content is available; please refresh.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible UX improvements:
- add link which user can click to reload page:
New content is available, <a onclick="window.location.reload()" >please refresh</a>.
- Do not show message about reload and instead reload browser when user navigates to the next page. But this makes sense only if app actually has routes and uses something like react-router
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#2755 closed BTW
As this PR got somehow stuck, I've decided to prepare an other one, dealing just with callbacks inside registerServiceWorker file: #3375 I'm using this technique for few months and I'm happy with it. |
}, | ||
this.props.timeout | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better adding space between functions?
Closing as part of #4169; I'd love to see this released as a user-land feature though we can add to the documentation. Thanks for all your efforts! |
Added Toast component for Service Worker, inspired by #2398
It only listens for first registration and update events. I tried to add one to indicate that the content is served from service worker, but it's annoying because it's always shown on every refresh.