Skip to content

Automatic desktop notifications on build errors - #134

Merged
filipesilva merged 1 commit into
angular:masterfrom
jkuri:notifications
Jan 24, 2016
Merged

Automatic desktop notifications on build errors#134
filipesilva merged 1 commit into
angular:masterfrom
jkuri:notifications

Conversation

@jkuri

@jkurijkuri commented Jan 4, 2016

Copy link
Copy Markdown
Contributor

I modified a TypeScript compiler to show notifications similar like grunt-notify.
This can come handy when developing an application.
Note that notifications are disabled by default and can be enabled in config/angular-cli.json file.

@jkurijkuri changed the title Show notifications on TypeScript build errorsAutomatic desktop notifications on TypeScript build errorsJan 4, 2016
@jkurijkuri changed the title Automatic desktop notifications on TypeScript build errorsAutomatic desktop notifications on build errorsJan 4, 2016
@filipesilva

Copy link
Copy Markdown
Contributor

I hadn't given much thought to this functionality but it seems to be useful and unobtrusive on the rest of the project. Plus since the cli is taking care of notifications, it's hard to sometimes know from within the editor wether or not the build is broken.

So lgtm. Could you rebase on top of master?

@jkuri

Copy link
Copy Markdown
ContributorAuthor

@filipesilva sure, rebased.

@Brocco

Copy link
Copy Markdown
Contributor

This also looks pretty good, although instead of having the configuration values stored in a new config file, please add an attribute in the root object in package.json to store this information.

I would suggest a name to match this repo/package angular-cli and pull the value from that file.

Also I do not think it makes sense to have the audio notification turned on by default and it that'd make a good value to be pulled from the configuration.

package.json

{
"angular-cli": {
"notifications": {
"enabled": true,
"playSound": false
}
}
}

@jkuri

Copy link
Copy Markdown
ContributorAuthor

@Brocco yes, you are right, there's no need for additional config file. Will modify it later tonight and make a commit.

@filipesilva

Copy link
Copy Markdown
Contributor

I hadn't even noticed the sound, but I'm totally with @Brocco on this one. It would drive me crazy.

@jkuri

Copy link
Copy Markdown
ContributorAuthor

@filipesilva haha. I made a commit as @Brocco suggested btw.

@filipesilva

Copy link
Copy Markdown
Contributor

@Brocco seems happy with it so I am happy to merge it. I noticed hansl was asking you on the other PR to squash the commits into one, and I forgot to ask you that myself.

To keep the history tidy, may I suggest you name the single commit feat(build): add desktop notifications ?

@jkuri

Copy link
Copy Markdown
ContributorAuthor

Done. Added name to commit as you suggested.

filipesilva added a commit that referenced this pull request Jan 24, 2016
Automatic desktop notifications on build errors
@filipesilva
filipesilva merged commit 0ac1629 into angular:masterJan 24, 2016
@filipesilva

Copy link
Copy Markdown
Contributor

And it's in! Thanks @jkuri !

@jkuri

Copy link
Copy Markdown
ContributorAuthor

np, thank you.

@jkuri
jkuri deleted the notifications branch January 24, 2016 02:33
@hansl

Copy link
Copy Markdown
Contributor

This PR will be reverted when we upgrade the broccoli files to the latest ones from angular/angular. If you want to add new features to the standard broccoli flow, do it in angular-app.js or do it in angular/angular.

filipesilva added a commit to filipesilva/angular-cli that referenced this pull request Feb 25, 2016
In angular#214, the main notification functionality was removed
due to it being in a auto-generated file.
This PR removes the remainder code that was left in from angular#134.
The `angular-cli` property in generated `package.json` is left in
since that was the place agreed for further configuration.
@angular-automatic-lock-bot

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-botangular-automatic-lock-botBot locked and limited conversation to collaborators Sep 9, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@jkuri@filipesilva@Brocco@hansl