Skip to content

Make status summary responsive, relating issue #1025#1046

Merged
mperham merged 6 commits intosidekiq:masterfrom
manishval:tweak-dashboard-status-ui
Jul 21, 2013
Merged

Make status summary responsive, relating issue #1025#1046
mperham merged 6 commits intosidekiq:masterfrom
manishval:tweak-dashboard-status-ui

Conversation

@manishval
Copy link
Contributor

Based on the issue #1025 and the last design.

@mperham
Copy link
Collaborator

mperham commented Jul 8, 2013

Screen shot?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0%) when pulling f351555 on manishval:tweak-dashboard-status-ui into 8410f99 on mperham:master.

@manishval
Copy link
Contributor Author

@mperham

This is the desktop view: http://cl.ly/image/1G3P313r1O35
This is the mobile view: http://cl.ly/image/1j3s0f3G3B1P

@mperham
Copy link
Collaborator

mperham commented Jul 8, 2013

@manishval You can just drop images right onto the text editor, then they will display inline.

@mperham
Copy link
Collaborator

mperham commented Jul 8, 2013

The mobile link doesn't work.

@manishval
Copy link
Contributor Author

@mperham I think CloudApp is down. Here they are though:

Desktop

sidekiq-live-poll-location

Mobile

sidekiq-mobile

@mperham
Copy link
Collaborator

mperham commented Jul 11, 2013

How about a mobile view which isn't a single column of numbers?

@manishval
Copy link
Contributor Author

@mperham so display the summary how it is right now with the two columns?

@mperham
Copy link
Collaborator

mperham commented Jul 11, 2013

Two columns would look better.

On Thu, Jul 11, 2013 at 3:37 PM, manishval notifications@github.com wrote:

@mperham https://github.com/mperham so display the summary how it is
right now with the two columns?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1046#issuecomment-20847672
.

@manishval
Copy link
Contributor Author

@mperham Here is the mobile version with 2 columns. Let me know if this is acceptable and I will update the pull request.

sidekiq-mobile-2

@mperham
Copy link
Collaborator

mperham commented Jul 20, 2013

Yeah, that looks much better.

@manishval
Copy link
Contributor Author

I have added the updated commit.

@mperham
Copy link
Collaborator

mperham commented Jul 20, 2013

As you shrink the window, the stat bar wraps early.

screen shot 2013-07-20 at 10 05 29 am

@mperham
Copy link
Collaborator

mperham commented Jul 20, 2013

Would you move the black info bar to the bottom of the mobile view?
screen shot 2013-07-20 at 10 06 46 am

@mperham
Copy link
Collaborator

mperham commented Jul 20, 2013

Aside from those two minor issues, it looks great. Don't forget to update the changelog to give yourself credit.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0%) when pulling f0a6545 on manishval:tweak-dashboard-status-ui into 8410f99 on mperham:master.

@manishval
Copy link
Contributor Author

@mperham I fixed the two minor issues and made the graphs responsive as well.

Whenever the page is loaded, we get the width of the summary bar since its width is fluid and render the graphs with that width. And whenever the window is resized the graphs are deleted from the page and re-drawn onto the page with the width of the summary bar. A delay has been added to the window resize event so the graphs aren't being reset every pixel. Not sure how suitable this method is.

@manishval
Copy link
Contributor Author

There is a cleaner way to make the graphs responsive. If the width attribute is not specified, then the graphs take the width of their parent container (which is responsive via twitter bootstrap). So I can get rid of the following code from my previous commit.

var responsiveWidth = function() {
  return document.getElementsByClassName('summary_bar')[0].clientWidth;
};

But as I mentioned I don't know if there is a better way to resize the graphs without re-rendering them.

Let me know if we should wait till we get a better way to resize the graphs or if I can go ahead and make a commit with the above code and the graphs width attribute removed.

@mperham
Copy link
Collaborator

mperham commented Jul 21, 2013

This looks fantastic. I'll merge as is but if you or @brandonhilkert have ideas to improve the graph rendering, just send a new PR.

mperham added a commit that referenced this pull request Jul 21, 2013
Make status summary responsive, relating issue #1025
@mperham mperham merged commit 95a57c2 into sidekiq:master Jul 21, 2013
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.

3 participants