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

Fix #43465 #44006

Merged
merged 3 commits into from Mar 7, 2018

Conversation

Projects
None yet
2 participants
@aushakou
Copy link
Contributor

aushakou commented Feb 20, 2018

This fixes #43465, removing shadow glitch under tab bar when minimap on the left side.
By the way, I wrote a blog post about fixing this issue.
I would appreciate any comments and suggestions
Thanks.

@alexandrudima

This comment has been minimized.

Copy link
Member

alexandrudima commented Mar 1, 2018

@aushakou Thank you for the PR.

This does not fix the issue, although it comes pretty close. The shadow element is placed before the minimap element in the DOM, and thus, the minimap is painted on top of the shadow element:

image

@aushakou

This comment has been minimized.

Copy link
Contributor

aushakou commented Mar 1, 2018

@alexandrudima I would like to continue working on this issue if it is possible. But I need some clarification about how the shadow element should be placed.
Should it be painted on top of the minimap? Or beside the minimap?
Thanks.

@alexandrudima

This comment has been minimized.

Copy link
Member

alexandrudima commented Mar 2, 2018

We could rearrange the dom nodes in https://github.com/Microsoft/vscode/blob/ab7f6e871d1af714ae454a06e2745a6ec0521704/src/vs/editor/browser/view/viewImpl.ts#L227:L234

But now I am having second thoughts if we should do this change at all:

Here is the case where scrolling to the right, the minimap also drops a shadow, and the top shadow and the right shadow form a nice corner at the minimap boundary. With this change things would not line up:

image

@aushakou

This comment has been minimized.

Copy link
Contributor

aushakou commented Mar 3, 2018

@alexandrudima From my understanding, if the minimap is on the right side, everything seems to be fine. If the minimap is on the left side, there are two issues.
Issue 1: shadow on the top is too short.
Issue 2: there is no shadow on the right side.

scr-vscode

This PR seems to fix issue 1 (the top shadow) by increasing the width of the shadow.
Issue 2 (shadow on the right side) is still unresolved.

scr-vscode-f

I think we should add the shadow on the right side by sticking it to the vertical scrollbar. And I think it might require a separate issue.

@alexandrudima

This comment has been minimized.

Copy link
Member

alexandrudima commented Mar 5, 2018

@aushakou Sorry about that. I somehow misunderstood the issue, and didn't place the minimap to the left.


May I suggest we do the following:

When the minimap is to the left, we make the right-hand side area of the editor behave as if the minimap is disabled. Namely:

  • the scrollbar should be transparent
  • the top shadow should extend over the scrollbar
    image

  • The fact that the minimap is enabled and placed to the left could be detected from the EditorLayoutInfo with layoutInfo.minimapWidth > 0 && layoutInfo.minimapLeft === 0

  • Make the scrollbar transparent in this case in decorationsOverviewRuler.ts

  • Make the shadow extend over the scrollbar in scrollDecoration.ts

aushakou added some commits Mar 6, 2018

Top shadow fix
The top shadow extends over the scrollbar
when minimap is on the left side or
when minimap is disabled.
The scrollbar transparency fix
The scrollbar is transparent when
minimap is on the left side.
@aushakou

This comment has been minimized.

Copy link
Contributor

aushakou commented Mar 6, 2018

@alexandrudima I tried to fix this issue by following your suggestions and it seems to work fine.
When the minimap is on the left side:

minimap-left

When the minimap is on the right side:

minimap-right

When the minimap is disabled:

no-minimap

@alexandrudima alexandrudima added this to the March 2018 milestone Mar 7, 2018

@alexandrudima

This comment has been minimized.

Copy link
Member

alexandrudima commented Mar 7, 2018

Nice! Thank you for your contribution! ❤️

@alexandrudima alexandrudima merged commit b166331 into Microsoft:master Mar 7, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
license/cla All CLA requirements met.
Details
@aushakou

This comment has been minimized.

Copy link
Contributor

aushakou commented Mar 7, 2018

@alexandrudima Thanks for your guidance and suggestions.

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