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

Clarify terminal is child window of details panel. #1903

Merged
merged 5 commits into from
Oct 25, 2016

Conversation

foot
Copy link
Contributor

@foot foot commented Oct 5, 2016

So when you close the details panel and the terminal closes too its not a surprise!

Fixes #1152

@rade
Copy link
Member

rade commented Oct 5, 2016

screen cap?

@foot
Copy link
Contributor Author

foot commented Oct 5, 2016

@rade
Copy link
Member

rade commented Oct 5, 2016

any chance we could change the 'close' to a 'slide in'?

@rade
Copy link
Member

rade commented Oct 5, 2016

and perhaps change the icon too, e.g. ">>"

@foot
Copy link
Contributor Author

foot commented Oct 5, 2016

>> / slide in suggests some kind of permanence/continuity to me, as in, I'm done with this terminal session for now, lets tuck it away for later.

I like: boom, its gone, gotta make a new one now.

Though you could interpret the idea of the having access to the container as continuous thing here and it would make more sense summoning it back, thoughts? (+ @davkal)

@rade
Copy link
Member

rade commented Oct 5, 2016

I see your point.

My concern is that I see the "x" button and "boom, its gone" action as indicators that the terminal has an independent life from the detail panel.

@foot
Copy link
Contributor Author

foot commented Oct 5, 2016

They feel quite connected now.

There are other onClose animations we could look into but I don't wanna make it too animation heavy, and I think a snappy close is usually nice: Done here, I'm off to do something else. Vs establishing context etc as we show something new.

Alt close animation:

  • Keep x
  • fade out + sliding back, signifying destruction + going back home to its parent the details panel.

@davkal
Copy link
Contributor

davkal commented Oct 18, 2016

I'm happy with the immediate close on X.

screen shot 2016-10-18 at 14 46 30

Not so happy with the aesthetic. Both panels look a bit bolted together, colors are clashing a bit. There is no clear hierarchy (other than suggested by the animation). Most oddly, the details panel has a shadow, but the terminal has not.

Counter-proposal: remove the dark color border, add padding (to make it smaller to suggest hierarchy), and add shadow.

screen shot 2016-10-18 at 14 46 12

@@ -328,4 +353,10 @@ class Terminal extends React.Component {
}
}


Terminal.defaultProps = {
connect: true

This comment was marked as abuse.

This comment was marked as abuse.

&-animation-wrapper {
width: 100%;
height: 100%;
transition: transform 0.5s cubic-bezier(0.230, 1.000, 0.320, 1.000);

This comment was marked as abuse.

@davkal davkal assigned foot and unassigned davkal Oct 18, 2016
@davkal
Copy link
Contributor

davkal commented Oct 24, 2016

YES! I like the hover bit. It feels like the details panel owns the terminal. Well done. LGTM

@foot foot merged commit 06b83a4 into master Oct 25, 2016
@foot foot deleted the 1152-clarify-term-is-child-window branch October 25, 2016 10:40
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.

it's not obvious that terminals are extensions of the details panel
3 participants