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

Nicer, friendlier HipChat notification message #531

Merged
merged 3 commits into from Aug 19, 2013

Conversation

Projects
None yet
3 participants
@brendonrapp
Contributor

brendonrapp commented Aug 4, 2013

The current HipChat notification is, in a word, hideous.

This pull request:

  • separates the mashed-together blocks of text (spaces are our friends)
  • uses bold to help highlight and separate bits of the string
  • uses HipChat's HTML capability to make a link to the Errbit problem URL, rather than pasting the ugly long URL string in the body of the message
  • adds a counter for the number of occurrences of the issue
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 4, 2013

Coverage Status

Coverage increased (+0%) when pulling 1eed79c on brendonrapp:master into 8800475 on errbit:master.

coveralls commented Aug 4, 2013

Coverage Status

Coverage increased (+0%) when pulling 1eed79c on brendonrapp:master into 8800475 on errbit:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 7, 2013

Coverage Status

Coverage increased (+0%) when pulling f18a981 on brendonrapp:master into fe1b7f5 on errbit:master.

coveralls commented Aug 7, 2013

Coverage Status

Coverage increased (+0%) when pulling f18a981 on brendonrapp:master into fe1b7f5 on errbit:master.

@@ -9,7 +9,7 @@
- if any_github_repos? || any_bitbucket_repos?
%th Repository
- if any_notification_services?
%th Notification Service
%th Notify

This comment has been minimized.

@shingara

shingara Aug 19, 2013

Member

This change is not really usefull. I revert it after merge

@shingara

shingara Aug 19, 2013

Member

This change is not really usefull. I revert it after merge

This comment has been minimized.

@brendonrapp

brendonrapp Aug 19, 2013

Contributor

That wasn't intended to be included. My mistake.

I disagree about it not being useful, though. The words "NOTIFICATION SERVICE" make that column of the screen unnecessarily wide, constricting screen space for other columns that would actually use that space instead of just have extra empty space next to the notification icon.

However, that belongs in a separate pull.

@brendonrapp

brendonrapp Aug 19, 2013

Contributor

That wasn't intended to be included. My mistake.

I disagree about it not being useful, though. The words "NOTIFICATION SERVICE" make that column of the screen unnecessarily wide, constricting screen space for other columns that would actually use that space instead of just have extra empty space next to the notification icon.

However, that belongs in a separate pull.

shingara added a commit that referenced this pull request Aug 19, 2013

Merge pull request #531 from brendonrapp/master
Nicer, friendlier HipChat notification message

@shingara shingara merged commit 9e1f25c into errbit:master Aug 19, 2013

1 check passed

default The Travis CI build passed
Details
@shingara

This comment has been minimized.

Show comment
Hide comment
@shingara

shingara Aug 19, 2013

Member

Thanks, Seems better

Member

shingara commented Aug 19, 2013

Thanks, Seems better

shingara added a commit that referenced this pull request Aug 19, 2013

Extract i18n key in apps/index view
Extract i18n key to allow translation in future.
Back to 'Notification Service' instead of 'Notify' introduce on
[f18a981]

see #531
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment