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.

Sign up for GitHub

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

Jump to bottom

PT#142772825 - snapshottimeline #848

Merged
merged 16 commits into from
Jul 27, 2017
Merged

PT#142772825 - snapshottimeline #848

merged 16 commits into from
Jul 27, 2017

Conversation

AllenBW
Copy link
Member

@AllenBW AllenBW commented Jul 18, 2017 •
edited
Loading

This work is following the mock here: https://www.pivotaltracker.com/story/show/142772825

This pr adds the reusable (and rather well executed) timeline component to the sui in addition to the timeline in the snapshots components.

Mock

snapshotsview 1

Where we're at

timeline

@AllenBW AllenBW added the wip label Jul 18, 2017
@AllenBW AllenBW added this to the Sprint 65 Ending Jul 24, 2017 milestone Jul 18, 2017
@AllenBW AllenBW requested a review from chriskacerguis as a code owner July 18, 2017 20:52
@AllenBW AllenBW force-pushed the BZ/#142772825-snapshottimeline branch 2 times, most recently from a2892c3 to 3b19baf Compare July 19, 2017 19:19
@AllenBW
Copy link
Member Author

AllenBW commented Jul 19, 2017 •
edited
Loading

looks like something is still up wth status... travis passed 🤔 https://travis-ci.org/ManageIQ/manageiq-ui-service/jobs/255020128

@chriskacerguis chriskacerguis self-assigned this Jul 20, 2017
@AllenBW AllenBW force-pushed the BZ/#142772825-snapshottimeline branch 3 times, most recently from 08390c7 to 5ba087b Compare July 21, 2017 20:27
@AllenBW AllenBW changed the title [WIP] BZ#142772825 - snapshottimeline BZ#142772825 - snapshottimeline Jul 21, 2017
@AllenBW
Copy link
Member Author

AllenBW commented Jul 21, 2017

@Loicavenel @serenamarie125 @chriskacerguis for your 👁 and 👂 - 🌮 💃

@Loicavenel
Copy link

@AllenBW this look very promising, can you add a screenshot when you mouse over the Camera on the timeline, please?
Also, @serenamarie125 to decide here, but could we remove the "Snaphot Timeline" text on the left? it is using a lot of place while we know what we are looking at...

@AllenBW
Copy link
Member Author

AllenBW commented Jul 21, 2017

@Loicavenel the second image shows what happens when you mouse over a camera, camera gets a little larger. In lieu of guidance I followed default behavior of patternfly-timeline

@serenamarie125
Copy link

Looking good so far! I'd like to get some visual input on this before it is approved, as integrating a timeline view and list view on the same page is something new.

Additionally, the tooltip should have a bit more information. Check out the specs here: https://docs.google.com/document/d/1YBi1rk5qBHqyTVrx1uN4f_JLRbAUZ27uUngwgsqP7mE/

Unfortunately the design doc hasn't made it to the GH repo yet. Let's review at the SUI Sync this week to be sure we are all on the same page

@serenamarie125
Copy link

@chriskacerguis i think should should have a ux/review tag on it

@AllenBW
Copy link
Member Author

AllenBW commented Jul 24, 2017

@serenamarie125 thanks for the guidance! will do 🙇‍♀️

@AllenBW
Copy link
Member Author

AllenBW commented Jul 25, 2017

@serenamarie125 @Loicavenel updated pr to include event tooltips that activate on hover

@Loicavenel
Copy link

@AllenBW Tool tip is great.. but can we make timeline span across the whole windows.? Can we remove the title on the left?

@AllenBW
Copy link
Member Author

AllenBW commented Jul 25, 2017

@Loicavenel as requested, no more left title, larger graph, and for free, you'll get a responsive graph as well 😽

@AllenBW AllenBW force-pushed the BZ/#142772825-snapshottimeline branch 2 times, most recently from a565462 to 4d78c22 Compare July 26, 2017 12:14
@miq-bot
Copy link
Member

miq-bot commented Jul 26, 2017

This pull request is not mergeable. Please rebase and repush.

@AllenBW AllenBW force-pushed the BZ/#142772825-snapshottimeline branch from bee0211 to 5ee6cd2 Compare July 26, 2017 19:02
@serenamarie125
Copy link

@AllenBW 👏

1 - can you make the amount of space between the new horizontal rule & the top of the filter component equal to the amount of space between the bottom of the filter component and the next "line"?
2 - can you make the new "line" have the same visuals as the one below the filter component?
3 - is it possible to have the "view context" (blue box) on the timeline below be the last 7 days? Although I'd like @Loicavenel to confirm that makes sense. This would give the ability to see a visualization of the last 7 days but still be able to zoom in/out to more!

@AllenBW
Copy link
Member Author

AllenBW commented Jul 26, 2017

@serenamarie125 not entirely sure i understand, but is this closer?

screen shot 2017-07-26 at 4 16 19 pm

presently, default zoom spans the entirety of context of results, can't pre-set some subset 🙍

@AllenBW AllenBW force-pushed the BZ/#142772825-snapshottimeline branch from 5ee6cd2 to 209f529 Compare July 26, 2017 20:22
serenamarie125
serenamarie125 approved these changes Jul 26, 2017
View reviewed changes
Copy link

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

🎨 yes @AllenBW . As for the "view context" (blue box) on the timeline, if this isn't currently supported, I can put in an issue to update it!

I know that @Loicavenel has some questions on making the camera icons larger, i'm not sure if that is a possibility, but i do agree that it would look nicer.

@chriskacerguis
Copy link
Contributor

@AllenBW let me know when you make the icons bigger, and I will merge. Good work on this.

@chriskacerguis
Copy link
Contributor

@AllenBW it's a pretty big coverage drop (1.9%)...within our "tolerance" level for a green...do you think that you can do a bit to bring it up?

@Loicavenel
Copy link

I am the only one who think the camera icon looks really tiny in the middle of timeline bar?

@AllenBW
Copy link
Member Author

AllenBW commented Jul 26, 2017

@Loicavenel @serenamarie125 @chriskacerguis bigger icons

screen shot 2017-07-26 at 5 34 49 pm

@chriskacerguis added more tests, if they don't do much, can add some stuff to other places in another pr

@Loicavenel
Copy link

@AllenBW good, size look better.. but I am scared you then removed the Zoom in effect?

@AllenBW
Copy link
Member Author

AllenBW commented Jul 26, 2017

@Loicavenel on hover no longer changes the icon size, is that what you mean by zoom effect?

@AllenBW
Copy link
Member Author

AllenBW commented Jul 26, 2017

screen shot 2017-07-26 at 6 00 51 pm

@Loicavenel can add it back if ya want

@Loicavenel
Copy link

really cool @serenamarie125 Approve please :)

@AllenBW AllenBW force-pushed the BZ/#142772825-snapshottimeline branch 3 times, most recently from 5b66be0 to a33210c Compare July 26, 2017 22:15
@serenamarie125
Copy link

i already approved, thanks for all the rework @AllenBW looks awesome !

@AllenBW AllenBW force-pushed the BZ/#142772825-snapshottimeline branch from a33210c to 80c96d1 Compare July 26, 2017 23:38
@AllenBW
Copy link
Member Author

AllenBW commented Jul 26, 2017

Yay! go team! 🌮 💃

@miq-bot
Copy link
Member

miq-bot commented Jul 26, 2017

Checked commits AllenBW/manageiq-ui-service@c9bbe17~...80c96d1 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. 👍

@chriskacerguis chriskacerguis merged commit c711ab7 into ManageIQ:master Jul 27, 2017
@AllenBW AllenBW deleted the BZ/#142772825-snapshottimeline branch July 27, 2017 12:15
@chriskacerguis chriskacerguis changed the title BZ#142772825 - snapshottimeline PT#142772825 - snapshottimeline Oct 15, 2017
@chriskacerguis
Copy link
Contributor

BZ - https://bugzilla.redhat.com/show_bug.cgi?id=1502318

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

@chriskacerguis chriskacerguis chriskacerguis approved these changes

@serenamarie125 serenamarie125 serenamarie125 approved these changes

@Loicavenel Loicavenel Awaiting requested review from Loicavenel

Assignees

@chriskacerguis chriskacerguis

Labels
enhancement ux/approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AllenBW @Loicavenel @serenamarie125 @miq-bot @chriskacerguis