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

Allow user to receive notification #39

Merged
merged 4 commits into from
Feb 15, 2017
Merged

Conversation

takai
Copy link
Contributor

@takai takai commented Feb 13, 2017

Allow user to receive a notification when the job is finished.

UI

screen shot 2017-02-13 at 19 23 57

Example

screen shot 2017-02-13 at 19 23 12

if (Notification.permission === 'granted' && Cookies.get('notification') === 'on') {
var notification = new Notification(
"[" + status + "] " + name,
{"icon": window.location.origin + "/assets/kuroko2/kuroko-logo-" + status.toLowerCase() + ".png"}
Copy link
Member

Choose a reason for hiding this comment

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

It is not necessarily to mount kuroko2 engine on root.
How about using the rails generated path or URL in any data attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assets within an engine are put into /assets path. This code will work.

Copy link
Member

Choose a reason for hiding this comment

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

Oops... I'm sorry, what I pointed out is mistake 😢
But the same situation will occur using config.assets.prefix or config.action_controller.asset_host, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your are right. I will change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed 7ecc693

Copy link
Member

@eisuke eisuke left a comment

Choose a reason for hiding this comment

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

LGTM

@eisuke eisuke merged commit a956c9a into cookpad:master Feb 15, 2017
@takai takai deleted the web_notifications branch February 15, 2017 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants