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

Some improvements to the error pages #203

Merged
merged 6 commits into from
Aug 1, 2016
Merged

Conversation

@Chocksy Chocksy force-pushed the error-display-improvements branch from 945fec9 to 4479e95 Compare July 19, 2016 08:50
@codecov-io
Copy link

codecov-io commented Jul 19, 2016

Current coverage is 97.03% (diff: 100%)

Merging #203 into master will increase coverage by <.01%

@@             master       #203   diff @@
==========================================
  Files           172        172          
  Lines          5553       5559     +6   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           5388       5394     +6   
  Misses          165        165          
  Partials          0          0          

Powered by Codecov. Last update 7fac6f5...1fb1450

@Chocksy
Copy link
Member

Chocksy commented Jul 27, 2016

Reviewed 3 of 4 files at r1.
Review status: 3 of 4 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


app/views/errors/index.html.haml, line 23 [r1] (raw file):

  #grouped-issuescontainer.col-lg-12.pagecontainer
    - subscribers = @errors.includes(:subscribers).map{ |x| { x.id => x.subscribers.size } }.reduce( { }, :merge)
    = render partial: 'error', collection: @errors, locals: { subscribers: subscribers }

Why are you doing this here? You can just have that subscribers call in the partial. Also, that line of code is really weird.


Comments from Reviewable

@hyperionel hyperionel force-pushed the error-display-improvements branch 2 times, most recently from ed2f8d0 to 91d7184 Compare July 28, 2016 10:31
- if subscribers.size > 0
%h5.pull-right
This message will be sent to
= subscribers.size.to_s + (subscribers.size > 1 ? ' users' : ' user')
Copy link
Member

Choose a reason for hiding this comment

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

Use i18n for this. Look at rails i18n count. You should have this line here: = t('subscribers.count', count: subscribers.size)

@Chocksy
Copy link
Member

Chocksy commented Aug 1, 2016

Reviewed 4 of 7 files at r3.
Review status: 6 of 10 files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

hyperionel added a commit that referenced this pull request Aug 1, 2016
@Chocksy
Copy link
Member

Chocksy commented Aug 1, 2016

:lgtm:


Reviewed 1 of 2 files at r2, 3 of 7 files at r3, 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@Chocksy Chocksy force-pushed the error-display-improvements branch from 98a853b to 1fb1450 Compare August 1, 2016 12:53
@Chocksy Chocksy merged commit 1fb1450 into master Aug 1, 2016
@Chocksy Chocksy deleted the error-display-improvements branch August 1, 2016 13:09
@Chocksy Chocksy temporarily deployed to epiclogger-staging August 1, 2016 13:13 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants