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

Visually hint that you can drag the control bar to the other side #732

Closed
wants to merge 2 commits into from

Conversation

samhed
Copy link
Member

@samhed samhed commented Dec 10, 2016

It's not obvious, so I thought I would add a hint for it.

@samhed samhed added the feature label Dec 10, 2016
@samhed
Copy link
Member Author

samhed commented Dec 10, 2016

screenshot from 2016-12-10 21-37-53

How it looks while moving the control bar handle, while the control bar is open.

Copy link
Member

@CendioOssman CendioOssman left a comment

Choose a reason for hiding this comment

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

This screws up the shadow. Most noticeable when it is on the left side and is closed.

background-color: rgb(83, 99, 122);
background-image: url("../images/handle_bg.svg");
background-repeat: no-repeat;
background-position: right;
box-shadow: 3px 3px 0px rgba(0, 0, 0, 0.5);
}
#noVNC_control_bar_handle:hover {
box-shadow: 0px 0px 7px 3px rgba(255, 255, 255, 0.6);
}
Copy link
Member

Choose a reason for hiding this comment

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

This removes the original shadow and just looks weird. Why this hover effect?

Copy link
Member

Choose a reason for hiding this comment

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

I assume this is also what causes the outline to be asymmetrical? Any specific reason for that? I think it looks odd.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I added it in the first place was to emphasize that it could be interacted with.. But now as I think more about it, I don't mind removing this.

Another reason to remove it is how poorly hovers work on mobile.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed it.

if (handleHint === null || controlBarHint === null) return;

handleHint.style.display = "none";
controlBarHint.style.display = "none";
Copy link
Member

Choose a reason for hiding this comment

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

We did a lot of work to get stuff like this out of the code an in to CSS. It would be a shame if we regress on that.

Any reason why you cannot put the look of the hint in CSS and just let JavaScript control when it is visible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, in most cases. Some of the style manipulation has to be done in JavaScript since the silhouettes needs the dimensions and positions of the target elements.

@CendioOssman
Copy link
Member

An animation to fade the hint in and out would be nice as well. :)

Instead of having it stick out on both sides, let's make it as wide as
it appears.
The control bar can be dragged to the other side, this isn't obvious
however. This adds a hint on the opposite side in the form of a
silhouette of the control bar.
@samhed
Copy link
Member Author

samhed commented Dec 12, 2016

Good points!

I fixed the shadow (with a pseudo element), added a fade-in and out animation and moved as much of the style manipulation as possible to css.

I'll probably make a new PR with another alternative, a more simple hint. I'm personally not 100% sold on this one due to the complexity of the solution and the questionable value.

@DirectXMan12 DirectXMan12 added this to the v0.7.0 milestone Dec 13, 2016
@samhed samhed removed this from the v0.7.0 milestone Mar 14, 2017
@samhed
Copy link
Member Author

samhed commented Mar 14, 2017

I'm going to close this one, I don't like this approach.

@samhed samhed closed this Mar 14, 2017
@samhed samhed deleted the visualhint branch March 14, 2017 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants