pcottle / learnGitBranching

An interactive git visualization and tutorial. Aspiring students of git can use this app to educate and challenge themselves towards mastery of git!
https://pcottle.github.io/learnGitBranching/
MIT License
30.77k stars 5.77k forks source link

Make it possible to keep the goal window visible (and maybe movable, minimizable, and restorable) #156

Closed lukas-shawford closed 10 years ago

lukas-shawford commented 10 years ago

First of all, really nice job on this - this is a great way to learn and become better at git. If I wanted to show someone how to learn git, this is exactly where I would send them.

My biggest gripe right now is the amount of effort that's required to bring up the "Goal to Reach" window, as well as the fact that there's no way to keep it open while you're working on the level. At the very least, I wish there was an easy, discoverable way to bring it up with a single click.

But ideally, I'd love to see the UI more functional as a whole. You have these containers styled as OS X windows, but they're aren't movable or resizable at the moment. If they were, that would be a great way to achieve this - you could have a menu bar at the bottom where you could minimize or restore all these windows as needed. If we go this route, then this issue is basically just a duplicate of #85.

Otherwise, just going the simpler route, I would recommend that the goal window should be on the right side (so it doesn't cover up the terminal window), and when it's closed, it doesn't completely disappear out of view, but simply collapses into the sidebar - perhaps similarly to how the sidebar on http://colourco.de/ works (though maybe use click instead of hover to bring it back). Clicking on the sidebar can basically just trigger the "show goals" or "hide goals" command.

I'd be willing to try to pitch in if you like either of the ideas above.

pcottle commented 10 years ago

Hey Sergey, thanks for the feedback!

I'm surprised the goal window seems so hard to bring up. Do you not see it open automatically when you start a level? It should show since the "show goal" command is automatically added when a new level starts.

You can click to dismiss the window easily if you want. I tried to make the CSS styling such that it wouldn't cover up whatever command you are inputting, although its hard to see the output of the commands (since errors show up behind the window).

If I moved it to the right side, I'm worried it would then cover up your current repo visualization. There doesn't seem to be enough room :P So when choosing between all of these, I figured the best things to keep on screen were:

while dropping:

I could add a click target somewhere though that would be an easier way of opening the goal window. What do you think? What if there was a button in the header here?

screen shot 2014-01-19 at 1 13 31 pm

pgampe commented 10 years ago

A toggle button would be great.

But I also prefer if you could make the goal "window" moveable, such that the user can put it where he things it fits for the current level.

lukas-shawford commented 10 years ago

I'm surprised the goal window seems so hard to bring up. Do you not see it open automatically when you start a level? It should show since the "show goal" command is automatically added when a new level starts.

I see it, but after looking at it, the first thing I do is close it to get it out of the way so I could see the terminal. In fact, until you pointed it out just now, I didn't even realize the terminal was usable while the goal window is open!

Also, having to type out "show goal" is a lot of work by today's standards! Actually, it's not too bad now that I realize it's possible to still use the terminal while the goal window is showing - I was constantly closing it by clicking then reopening it by typing out "show goal" as I was trying this out. In any case, having a button to bring it back up quickly would make this even easier.

If I moved it to the right side, I'm worried it would then cover up your current repo visualization. There doesn't seem to be enough room :P

I haven't tried the really advanced levels yet - I'll play around with this some more. But from the exercises I've tried so far, it doesn't look like it ought to be a problem - especially if we make these windows movable.

I could add a click target somewhere though that would be an easier way of opening the goal window. What do you think? What if there was a button in the header here?

That would work great. It would also be a good idea to call it out visually when you hide the goal window (e.g., flash it).

jjshoe commented 10 years ago

+1 need a way to move this window as many request. We're visual creatures, which is why we are using this site anyways... right?

sidgan commented 10 years ago

I agree with sergkr, it would be a good idea to include a column of 3, the command prompt in one, the goal in the second and the slightly smaller version of the current state.

lukas-shawford commented 10 years ago

Sorry it took me so long, but I finally got a chance to play around with this. Check out my fork to see my progress so far:

https://github.com/sergkr/learnGitBranching

All terminal windows except for the main command window are now draggable:

learngitbranching-progress1

When the goal window is minimized, a "Goal" button shows up in the level toolbar to make it easier to bring it back up with a single click:

learngitbranching-progress2

Clicking it restores the goal window back to its previous position and size.

If you get a chance, play around and let me know what you think. If you spot any issues, let me know.

I think the next question ought to be what should the default placement be for the goal window - should we have a column of 3 like Siddha suggested, with the goal window on the right? That's kind of what I'm leaning toward myself. Or should we pop it up roughly where it is now, except now the user has the ability to drag it to the right side if they want?

Also, as the first screenshot demonstrates, perhaps we should squeeze the main visualization a bit so that you could keep the goal window on the right if you wanted to, and not have it overlap with the repo visualization. Maybe we should do that only if the goal window is visible? I.e., when you bring the goal window up, the main repo visualization shrinks a bit, and then when you minimize it, it grows again?

pgampe commented 10 years ago

Looks good to me. I would not play around too much with effects. The additional noise confuses the user.

I would even prefer if the Goal button would change from show goal to hide goal and be always visible. That way you could take a quick look at the goal without having to move your moves across all over the screen (fast hide/unhide).

sidgan commented 10 years ago

Working on the idea Philipp suggested, the goal window could be made visible and invisible alternately. When it says "Show Goal", the goal image appears below it and when "Hide Goal" the image disappears. Hence normally only a flying bar is seen with "Show Goal". Its position can be changed according to the user. When this is clicked the goal image appears below it and stays. Also the "Show Goal" changes to "Hide Goal". Now when "Hide Goal" is clicked the image disappears. This wont take up too much space and then one won't have to move the whole window repeatedly.

pcottle commented 10 years ago

Hey @sergkr thanks a bunch for whipping up a fork and trying this out! I could easily make the "base" visualization squeeze a bit so there would be room on the right for the goal.

However I'm concerned about window size here. At what resolution is the screenshot you linked? One of the things I did was try to support fairly narrow screens that are only a few hundred pixels wide: screen shot 2014-02-27 at 12 15 46 pm

Anyways, here is my brain dump:

So here is my proposition:

Does that sound good? I think the third bullet there will deliver the all-viewable experience you want without sacrificing the small screen experience

lukas-shawford commented 10 years ago

@pcottle, your proposition sounds good to me. I got some more work done on this today and have mostly implemented it over on my fork (in the draggable-windows branch). I made it so that if you drag the goal window into the right half of the screen, the main repo visualization shrinks a bit horizontally. It's actually kind of neat, I think.

I disabled the ability to drag other windows (like the tutorial windows and alerts), since there isn't much use for that anyway. The only downside is the lack of consistency (similar UI, but some windows are draggable, whereas others aren't). Maybe we can make this draggable feature a bit more discoverable by turning the cursor into a move cursor when hovering over the title bar? There's an option for that with jQuery UI draggable, so it would be pretty easy to do if we wanted to.

At the moment, there are a couple issues I'm currently still trying to work through. The first and most important is, on the remote levels, the resizing doesn't always work with the remote tree. This issue applies to both the main repo visualization, as well as the visualization within the goal window. When a resize gets triggered on either of these (which occurs when the browser window gets resized, and also when the goal window gets dragged or restored), the local git tree visualization gets resized properly, but the remote visualization doesn't. Example:

remote-resize-issue

I've been relying on Visualization.myResize, which eventually calls GitVisuals.refreshTree. I haven't looked into this all that thoroughly yet, but I think the issue here might be that refreshTree only considers the root commit of the local repo... I'll try to figure this out sometime over the next week or so, but if you have any recommendations on how to approach this, please let me know.

On an unrelated note, nice tree.

The next issue is, we're kind of tight on room in the level toolbar, and it's a bit hard to fit that Show Goal / Hide Goal button without making it ridiculously small. I used to have it just say "Goal", but now that there's more text, we start running into space issues. I decreased the font and the spacing on everything within the level toolbar, but long names (such as level remoteAdvanced5) can still be an issue, as in this screenshot:

goal-button-issue

Does anyone have any ideas about what we can do with this button?

Lastly, with the button in place, what are your thoughts on the current feature where clicking on the goal window itself hides it? The reason I ask is it's causing some conflicts with the draggable functionality. If you drag the goal window down all the way, but keep moving the mouse downward even more before releasing the mouse button, that will apparently cause the click event to fire, which hides the goal window. So it's easy to accidentally trigger closing the goal window when you meant to just drag it down. My own thoughts on how to deal with this is to just disable the ability to hide the goal window by clicking on it, since with the "Hide Goal" button in place, and also the "hide goal" command, we already have enough ways to close the goal window.

Let me know your thoughts on the above. I feel like we're getting pretty close now, but there's still a few more wrinkles to iron out. Hopefully, I'll have an actual pull request for this within a week or so, time permitting.

pcottle commented 10 years ago

I got some more work done on this today and have mostly implemented it over on my fork (in the draggable-windows branch)

Nice! Do you think this is ready for merge? I'm totally fine with doing some cleanup / bugfix work after we get the bulk of your work in

I disabled the ability to drag other windows (like the tutorial windows and alerts), since there isn't much use for that anyway

Totally agree. Yeah the inconsistency in the UI is something unfortunate, but maybe we could style the windows separately someday. They are modal dialogs anyway though, so there's not much use in dragging them

Maybe we can make this draggable feature a bit more discoverable by turning the cursor into a move cursor when hovering over the title bar?

Yeah this would be easy with CSS

The first and most important is, on the remote levels, the resizing doesn't always work with the remote tree

I'm not surprised by this at all -- the remote visualization code was a total hack. Basically every level / explanation dialog expected to have just one "visualization" object that would represent the one "repro" it was animating / modifying. The issue with remotes is that now we have two repos, and we need two visualizations. It was far too late to do all the refactoring work, so basically I hacked it by making the git repro construct another repro inside of itself (basically self-class composition). Same with the visualization object. This means you have to punt a bunch of commands over to the originVisualization manually -- super lame. Anyways, I'm totally down to solve this bug if you want to just commit. If not, try looking at Visualization.originToo: https://github.com/pcottle/learnGitBranching/blob/master/src/js/visuals/visualization.js#L122

It will punt over the command to the origin visualization (if necessary).

On an unrelated note, nice tree.

Haha thanks! Took me a while on caltrain

I used to have it just say "Goal", but now that there's more text, we start running into space issues.

I'll have to see how you are positioning the button, but I'd be fine with just floating the elements left / right and then clearfixing the surrounding container. Basically it'd look fine in 95% of cases and when the text gets too long, it'd simply render on a new line.

I'm assuming right now you have it absolutely positioned or something, hence the risk that text can get cut off. I'll inspect your CSS when it goes up :P

Lastly, with the button in place, what are your thoughts on the current feature where clicking on the goal window itself hides it?

Yeah, I'd probably scope this click event to just the body of the dialog. I can try to repro when I get your work merged in, but my best guess is that it's just a scoping issue. If not we can rely on the button

pcottle commented 10 years ago

Super excited to get this in @sergkr! Let me know if I should clone your repo and start merging or if you want to put up a pull request

:+1: :dancer:

lukas-shawford commented 10 years ago

Hey, sorry for taking so long to get back. I just sent a pull request with the work that I've done, along with a summary of the issues currently present.

I rebased my changes on top of the latest from your repo, and also squashed everything into one commit. It's probably cleaner that way since I had some backtracking on my old branch. However, let me know if you prefer the full history (I preserved it on the "draggable-windows-old" branch).

pcottle commented 10 years ago

Everyone give @sergkr a huge round of applause! :clap: :clap: :dancer: PR #166 has been merged into master. I followed up with a few commits to resolve the outstanding bugs and now draggable windows are in production proper. I encourage everyone on this thread to go try them out and followup if you find any bugs -- I completed a few windows and messed around but couldn't find anything horrendous.

Also everyone was right -- it's way nicer to solve levels while simultaneously looking at the command history / goal repo / current repo all in one view. I think I've been working on this project for too long and I'm blind to pretty obvious (in hindsight) UX issues. But thanks for pushing back and thanks to Sergey for putting up the PR.

Sergey do you mind liking the Facebook page at facebook.com/LearnGitBranching? I was going to post about this and tag you (I have some ad credits left over so I might as well do a little paid promotion)

sidgan commented 10 years ago

Definitely awesome! I tried out the all-new draggable windows and so far I don't have anything nasty to report. Great going @sergkr! :clap: :clap:

lukas-shawford commented 10 years ago

Thank you all, this has been a really fun one to work on.

@pcottle, I would, but I don't have a Facebook account at the moment. I'll spread the word by other means, though.

pcottle commented 10 years ago

No worries @sergkr and thanks one last time!