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

Fix #43465 #44006

Merged
merged 3 commits into from
Mar 7, 2018
Merged

Fix #43465 #44006

merged 3 commits into from
Mar 7, 2018

Conversation

aushakou
Copy link
Contributor

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.

@alexdima
Copy link
Member

alexdima 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
Copy link
Contributor Author

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.

@alexdima
Copy link
Member

alexdima 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
Copy link
Contributor Author

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.

@alexdima
Copy link
Member

alexdima 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 2 commits March 6, 2018 06:25
The top shadow extends over the scrollbar
when minimap is on the left side or
when minimap is disabled.
The scrollbar is transparent when
minimap is on the left side.
@aushakou
Copy link
Contributor Author

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

@alexdima alexdima added this to the March 2018 milestone Mar 7, 2018
@alexdima
Copy link
Member

alexdima commented Mar 7, 2018

Nice! Thank you for your contribution! ❤️

@alexdima alexdima merged commit b166331 into microsoft:master Mar 7, 2018
@aushakou
Copy link
Contributor Author

aushakou commented Mar 7, 2018

@alexandrudima Thanks for your guidance and suggestions.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tabbar shadow glitch when minimap on the left side
2 participants