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

Use service worker to display notifications if available #1580

Merged
merged 2 commits into from
Nov 19, 2017

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Sep 28, 2017

Displays notifications if the client is active on mobile devices (new Notification fails there)

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Sep 28, 2017
"use strict";

// TODO: This does not strip hex based colours
module.exports = (message) => message.replace(/\x02|\x1D|\x1F|\x16|\x0F|\x03(?:[0-9]{1,2}(?:,[0-9]{1,2})?)?/g, "").trim();
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I sure am glad we run production builds in our CI!

According to https://stackoverflow.com/questions/43888474/unexpected-token-operator-from-uglifyjs, Webpack relies on a version that doesn't support ES6 yet, so hopefully by the time this PR gets merged, they will be done (?).
I'm really surprised why this arrives only now though, since that's not the first fat-arrow function we use on the server...

Copy link
Member Author

Choose a reason for hiding this comment

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

That was because src folder wasn't included in babel transpiling.

Copy link
Member

Choose a reason for hiding this comment

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

Do we want to add the src though? I don't know, I feel odd about that. We have client code we use on the server and now server code we use on the client :/

Also does that mean more gets added to the client bundle? I compared and it doesn't seem to any extra kb heavier.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could move this file to client (and reference it on the server like we already do with findLinks) instead, this will at least keep the consistency in this regard.

Copy link
Member

Choose a reason for hiding this comment

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

That seems doing yeah

Copy link
Member

Choose a reason for hiding this comment

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

Fine*

Damn, 1.5month to notice my typo...

@astorije astorije added this to the 2.6.0 milestone Oct 1, 2017
@xPaw xPaw force-pushed the xpaw/mobile-active-notifications branch from 1bb3d5e to d7c6163 Compare October 1, 2017 07:36
@xPaw xPaw modified the milestones: 2.6.0, 2.7.0 Oct 19, 2017
"use strict";

// TODO: This does not strip hex based colours - issue #1413
module.exports = (message) => message.replace(/\x02|\x1D|\x1F|\x16|\x0F|\x03(?:[0-9]{1,2}(?:,[0-9]{1,2})?)?/g, "").trim();
Copy link
Member

Choose a reason for hiding this comment

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

You need to add this file to .npmignore now, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@astorije astorije force-pushed the xpaw/mobile-active-notifications branch from d7c6163 to d89c26a Compare November 19, 2017 06:44
@astorije
Copy link
Member

Rebased on master and fixed conflicts (ESLint stuff only).

@xPaw xPaw force-pushed the xpaw/mobile-active-notifications branch from d89c26a to 0402554 Compare November 19, 2017 16:20
@astorije astorije removed their assignment Nov 19, 2017
@xPaw xPaw merged commit 055bd5d into master Nov 19, 2017
@xPaw xPaw deleted the xpaw/mobile-active-notifications branch November 19, 2017 16:40
@AlMcKinlay AlMcKinlay removed their assignment Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants