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

Toast for Service Worker #2426

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions packages/react-dev-utils/ModuleScopePlugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,7 @@ class ModuleScopePlugin {
// Error if in a parent directory of src/
const requestRelative = path.relative(appSrc, requestFullPath);
if (
requestRelative.startsWith('../') ||
requestRelative.startsWith('..\\')
requestRelative.startsWith('../') || requestRelative.startsWith('..\\')
) {
callback(
new Error(
Expand Down
39 changes: 39 additions & 0 deletions packages/react-scripts/template/src/Toast.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
.Toast {
Copy link
Contributor

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 ?

Copy link
Contributor Author

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

position: fixed;
bottom: 0;
left: 0;
right: 0;
padding: 16px;
margin: auto;
transition: 250ms;
background: #222;
color: white;
line-height: 24px;
border-top-left-radius: 2px;
border-top-right-radius: 2px;
display: flex;
Copy link
Contributor

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?

}

.Toast.active {
transform: translateY(0);
}

.Toast.inactive {
transform: translateY(100%);
}

.Toast a,
.Toast-button {
Copy link
Contributor

@ro-savage ro-savage Jun 17, 2017

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?

padding: 0;
background: transparent;
border: 0;
color: #61dafb;
font-size: inherit;
line-height: inherit;
text-transform: uppercase;
text-decoration: none;
}

.Toast-text {
flex: 1;
}
42 changes: 42 additions & 0 deletions packages/react-scripts/template/src/Toast.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import React from 'react';
import './Toast.css';

export default class Toast extends React.Component {
state = {
active: true,
};

close = () => {
clearTimeout(this.timeout);
this.setState({
active: false,
});
};
componentDidMount() {
this.timeout = setTimeout(
() => {
this.setState({
active: false,
});
},
this.props.timeout
);
}

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?

componentWillUnmount() {
clearTimeout(this.timeout);
}
render() {
return (
<div className={`Toast ${this.state.active ? 'active' : 'inactive'}`}>
<span className="Toast-text">
{this.props.children}
</span>
<button className="Toast-button" onClick={this.close}>Dismiss</button>
</div>
);
}
}

Toast.defaultProps = {
timeout: 3000,

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.

Copy link
Contributor Author

@viankakrisna viankakrisna May 31, 2017

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

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.

};
45 changes: 42 additions & 3 deletions packages/react-scripts/template/src/registerServiceWorker.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,32 @@

// To learn more about the benefits of this model, read https://goo.gl/KwvDNy.
// This link also includes instructions on opting out of this behavior.
import React from 'react';
import ReactDOM from 'react-dom';
import Toast from './Toast';

const useToast = true;
const domId = 'toast';
let dom = document.getElementById(domId);

if (!dom) {
Copy link
Contributor

@ro-savage ro-savage Jun 17, 2017

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.

dom = document.createElement('div');
dom.id = domId;
document.body.appendChild(dom);
}

function showMessage(message) {
if (useToast) {
ReactDOM.render(
<Toast>
{message}
</Toast>,
dom
);
} else {
console.log(message);
}
}

const isLocalhost = Boolean(
window.location.hostname === 'localhost' ||
Expand Down Expand Up @@ -41,6 +67,19 @@ export default function register() {
}
});
}

if (process.env.NODE_ENV !== 'production') {
showMessage(
<span>
Development mode started.{' '}
<a
href="https://github.com/facebookincubator/create-react-app/blob/master/packages/react-scripts/template/README.md"
>
Read Me
</a>
</span>
);
}
}

function registerValidSW(swUrl) {
Expand All @@ -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.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible UX improvements:

  1. add link which user can click to reload page:
New content is available, <a onclick="window.location.reload()" >please refresh</a>.
  1. 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice idea! I am waiting for #2755 to get in so we can solve #2638 and continue working on SW enhancements

Choose a reason for hiding this comment

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

#2755 closed BTW

} 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.');
}
}
};
Expand Down Expand Up @@ -93,7 +132,7 @@ function checkValidServiceWorker(swUrl) {
}
})
.catch(() => {
console.log(
showMessage(
'No internet connection found. App is running in offline mode.'
);
});
Expand Down