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

shell: Zooming #1816

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
3 participants
@mvollmer
Copy link
Member

commented Feb 16, 2015

No description provided.

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2015

@andreasn, the icons need consideration, I'd say. Also, the whole interaction experience needs to be checked carefully.

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2015

@andreasn, the icons need consideration, I'd say.

And the colors and styling of the zoom box... so, pretty much everything. :-)

@mvollmer mvollmer added the needswork label Feb 16, 2015

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2015

Zooming should be disabled when there are no archives.

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2015

We should limit the zoom range. Minimum 5 minutes, say. Something goes wrong when you zoom out a lot.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2015

We should limit the zoom range. Minimum 5 minutes, say. Something goes wrong when you zoom out a lot.

Sounds like a good idea.

@mvollmer mvollmer force-pushed the mvollmer:metrics-zoom branch 2 times, most recently from c4a5616 to 7abff4a Feb 18, 2015

@mvollmer mvollmer removed the needswork label Feb 18, 2015

@mvollmer mvollmer force-pushed the mvollmer:metrics-zoom branch from 7abff4a to b8eb781 Feb 18, 2015

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2015

Got it running after we figured out the minified issue! \o/

Will need to play around with this some more, but looks like a good start.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2015

This yellow selection color would work better as blue, similar to what we use for other elements.
screenshot from 2015-02-18 16 16 58

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2015

None of the font-icon fonts have any good zoom out icon, so I'll fix up a custom icon

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2015

As @mvollmer noted on IRC, it's possible to zoom out to a point where the whole canvas goes blank, so it would be good to have a upper cap, like at a week.
screenshot from 2015-02-19 11 49 06

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2015

I'm also having some troubles getting a good sense of what the zoom out parameter is, and I don't think we came to any exact conclusion when we spoke about this in Brno.
I think it would either need to work as a reset of the zoom-in-action you just did, or mapped to one of the predefined time steps. Having a third one makes me lose direction of where I am right now.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2015

mvollmer referenced this pull request in andreasn/cockpit Feb 23, 2015

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2015

Ok, more IRC discussion with @andreasn has lead to this:

  • Dropdown instead of button group. "Go to now", separator, "5 minutes", "30 minutes", "1 hour", "6 hours", "1 week"
  • Cursor change on hover of graphs (to be drawn)
  • Dropdown shows some pleasantly rounded version of the current range
  • Zoom out is undo for zoom in. If there is no history, zoom out goes to next largest range from dropdown list. Scrolling (including "go to now") does not change zoom history. Direct zoom via dropdown item removes zoom history.

@mvollmer mvollmer added the needswork label Feb 23, 2015

@mvollmer mvollmer force-pushed the mvollmer:metrics-zoom branch 2 times, most recently from d58bed3 to 612d6a5 Feb 23, 2015

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2015

Improved icon here https://github.com/andreasn/cockpit/tree/metrics-zoom-icon

The actual icon is unfortunately missing...

@mvollmer mvollmer force-pushed the mvollmer:metrics-zoom branch 2 times, most recently from 4ea51ea to 49f99d1 Feb 24, 2015

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2015

@andreasn, this is ready for design re-review.

@mvollmer mvollmer force-pushed the mvollmer:metrics-zoom branch 2 times, most recently from 68f641f to 4e6ecde Feb 24, 2015

@mvollmer mvollmer force-pushed the mvollmer:metrics-zoom branch from 5333eb2 to 4dbeae6 Mar 3, 2015

@mvollmer mvollmer removed the needswork label Mar 3, 2015

@mvollmer mvollmer force-pushed the mvollmer:metrics-zoom branch from 4dbeae6 to bbc2a51 Mar 3, 2015

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2015

Good progress! This is getting closer!
A couple of small items:

  • The time picker is missing a little dropdown arrow.
  • The buttons grows and shrinks depending on the content
  • 1 month sounds better than one week and "about 1 year" sounds better than 51 weeks, at least for these two preset steps. I can see how it could be hard to calculate though, so might be all right anyway.

@mvollmer mvollmer force-pushed the mvollmer:metrics-zoom branch from bbc2a51 to 9572b11 Mar 4, 2015

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2015

1 month sounds better than one week and "about 1 year" sounds better than 51 weeks, at least for these two preset steps. I can see how it could be hard to calculate though, so might be all right anyway.

No, my pseudo-scientific mind just wasn't happy that "one month" and "one year" is not a fixed number of seconds, unlike "one week" (modulo DST and leap seconds, and...). But it's all approximate anyway, so we can just use 1 month = 30 days and 1 year = 365 days.

@mvollmer mvollmer force-pushed the mvollmer:metrics-zoom branch from 9572b11 to f8ebe1b Mar 4, 2015

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2015

A couple of small items:

Fixed!

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2015

Yay! This works really well now! Thanks for all the hard work!
Final nit is that the label is centered, and we don't do this anywhere else in this kind of control. Apart from that should be good to go!

@mvollmer mvollmer force-pushed the mvollmer:metrics-zoom branch from f8ebe1b to 3de4029 Mar 5, 2015

@andreasn andreasn removed the needsdesign label Mar 6, 2015

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2015

Fixed the alignment of the dropdown here andreasn@ffba8a0

@stefwalter stefwalter assigned mvollmer and unassigned andreasn Mar 11, 2015

@mvollmer mvollmer force-pushed the mvollmer:metrics-zoom branch from 3de4029 to aadf2c6 Mar 13, 2015

@mvollmer mvollmer removed their assignment Mar 13, 2015

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2015

Ready for review.

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2015

I installed this ... I see the cursor <->, and drag on the dashboard graph, but nothing happens. No javascript error either.

@mvollmer mvollmer force-pushed the mvollmer:metrics-zoom branch from aadf2c6 to 64f484d Mar 16, 2015

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2015

I installed this ... I see the cursor <->, and drag on the dashboard graph, but nothing happens. No javascript error either.

Hmm, the minimum range is 5 minutes, and if you try to zoom in below that, it will just stay at 5 minutes. Is that what you see?

Can you zoom out, via the zoom-out button or the menu?

In any case, we should not offer to zoom when we have reached the minimum, that is just confusing. Good point.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2015

In any case, we should not offer to zoom when we have reached the minimum, that is just confusing.

Agreed.

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2015

Continued in #2020.

@mvollmer mvollmer closed this Mar 25, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.