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

Clear the superfluous scrollbars at the DebugConfigurationWidget and DebugToolBar #5879

Merged
merged 1 commit into from
Aug 9, 2019
Merged

Conversation

xieyuanhuata
Copy link
Contributor

@xieyuanhuata xieyuanhuata commented Aug 7, 2019

What it does

fix #5819 - Clear the superfluous scrollbars for debug widgets

before:
62140406-3a7f7100-b2eb-11e9-9f69-bde4d4729418

after:
after

How to test

Open debug
Moves the mouse to the right of the DebugConfigurationWidget or DebugToolBar border

Review checklist

Reminder for reviewers

@xieyuanhuata
Copy link
Contributor Author

@AlexTugarev please try whether it behaves as you expect

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

It works really well, I no longer see the superfluous scrollbar from the debug configuration and toolbar widgets. Nice work 👍

@akosyakov akosyakov added the debug issues that related to debug functionality label Aug 7, 2019
@@ -51,6 +51,11 @@ export class DebugConfigurationWidget extends ReactWidget {
this.toDispose.push(this.manager.onDidChange(() => this.update()));
this.toDispose.push(this.workspaceService.onWorkspaceChanged(() => this.update()));
this.toDispose.push(this.workspaceService.onWorkspaceLocationChanged(() => this.update()));
this.scrollOptions = {
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks @xieyuanhuata for fixing this issue!

I'm generally fine with disabling perfect scroll bar as as, but why should it be created in the first place?
I think unsetting it in ctor like this would also to the job. @akosyakov is there a use for the scrollbar at all in those two widgets?

    constructor() {
        super();
        this.scrollOptions = undefined;
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AlexTugarev I agree with your opinion

Copy link
Member

Choose a reason for hiding this comment

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

no, unsetting like @AlexTugarev suggested should work as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AlexTugarev @akosyakov
you are right! Unsetting like @AlexTugarev suggested work as well.

@akosyakov
Copy link
Member

@vince-fugnitto @AlexTugarev when you are fine please help with landing @xieyuanhuata don't have write access

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I confirmed that it also works correctly to unset the scrollbar for the debug config and toolbar widgets.
Just as an FYI you can add the following line:

this.scrollOptions = undefined

Directly in the:

@postConstruct()
protected init(): void { ... }

@xieyuanhuata
Copy link
Contributor Author

@vince-fugnitto I think the effect is the same in ctor and init(). Maybe init is better

@vince-fugnitto
Copy link
Member

@vince-fugnitto I think the effect is the same in ctor and init(). Maybe init is better

It's up to you, both work fine :) (init would mean less code)

Signed-off-by: xieyuanhu <xieyuanhuata@163.com>
@akosyakov akosyakov merged commit 441ce8e into eclipse-theia:master Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug issues that related to debug functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[debug] Superfluous scrollbars
4 participants