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.

Sign up for GitHub

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

Jump to bottom

View container layout changes #5536

Merged
merged 2 commits into from
Jul 5, 2019
Merged

View container layout changes #5536

merged 2 commits into from
Jul 5, 2019

Conversation

spoenemann
Copy link
Contributor

Fixes #5175. This is a continuation of #5392.

@spoenemann spoenemann mentioned this pull request Jun 20, 2019
Closed
@spoenemann spoenemann force-pushed the GH-5175b branch 7 times, most recently from 2679e38 to 8dea8fd Compare June 25, 2019 12:33
@spoenemann spoenemann marked this pull request as ready for review June 25, 2019 12:34
@kittaakos
Copy link
Contributor

I am trying this now. Please note the linter error; the CIs are red.

@kittaakos
Copy link
Contributor

Hide > Show does not respect the previous state, but always expands the view container part:
screencast 2019-06-26 10-34-12

kittaakos
kittaakos reviewed Jun 26, 2019
View reviewed changes
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

I have checked the code and the behavior as well: it worked very reliably 👍. However, the collapse/expand behavior is slightly different from the VS Code one.

Theia:
screencast 2019-06-26 11-52-44

VS Code:
screencast 2019-06-26 11-53-40

packages/core/src/browser/view-container.ts Outdated Show resolved Hide resolved
packages/core/src/browser/view-container.ts Outdated Show resolved Hide resolved
packages/core/src/browser/view-container.ts Outdated Show resolved Hide resolved
packages/core/src/browser/view-container.ts Outdated Show resolved Hide resolved
packages/core/src/browser/view-container.ts Outdated Show resolved Hide resolved
@spoenemann
Copy link
Contributor Author

Hide > Show does not respect the previous state, but always expands the view container part

I did that intentionally. I thought when I "unhide" a part I want to see its content, too. Do you think we should change that?

@spoenemann
Copy link
Contributor Author

the collapse/expand behavior is slightly different from the VS Code one.

Yes, that's due to the default fit behavior of SplitLayout, which distributes the gained space across all expanded parts. I think it's not bad.

@kittaakos
Copy link
Contributor

Do you think we should change that?

If it was on purpose; no.

@akosyakov
Copy link
Member

@spoenemann Could you address @kittaakos comments and build failures please?

@spoenemann
Copy link
Contributor Author

Done.

@akosyakov
Copy link
Member

akosyakov commented Jul 5, 2019 •
edited
Loading

Something wrong with the first section of the debug view, there are scrolls:
Screen Shot 2019-07-05 at 10 33 25
Screen Shot 2019-07-05 at 10 33 23

@spoenemann can you have a look at it?

Rather than that it is super good change. Cannot wait to integrate it in the plugin system.

akosyakov
akosyakov approved these changes Jul 5, 2019
View reviewed changes
Copy link
Member

@akosyakov akosyakov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

please fix bogus scrolls and merge

also please update the changelog

@akosyakov akosyakov added shell issues related to the core shell vscode issues related to VSCode compatibility labels Jul 5, 2019
@akosyakov
Copy link
Member

akosyakov commented Jul 5, 2019 •
edited
Loading

We should revisit how we contribute toolbar items. We should do it via menu contributions, i.e. there should be new menu path for each view container part which is displayed as a toolbar. It would match to VS Code better and does not require new interfaces as now. But it can be done as a separate PR.

@spoenemann
Copy link
Contributor Author

I don't have time to finish this before my vacation.

@kittaakos
Copy link
Contributor

I don't have time to finish this before my vacation.

I'll take care of it later today.

Akos Kitta and others added 2 commits July 5, 2019 12:37
GH-5175: Initial view container layout changes.
f05083a 
Signed-off-by: Akos Kitta <kittaakos@typefox.io>
@spoenemann
GH-5175: Finished view container layout changes
d4332c6 
Signed-off-by: Miro Spönemann <miro.spoenemann@typefox.io>
@kittaakos
Copy link
Contributor

I'll take care of it later today.

It's done.

@akosyakov
Copy link
Member

Actually i see scrolls on master as well, please merge.

@kittaakos kittaakos merged commit 5f91fb5 into master Jul 5, 2019
@kittaakos kittaakos deleted the GH-5175b branch July 5, 2019 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kittaakos kittaakos kittaakos left review comments

@akosyakov akosyakov akosyakov approved these changes

@svenefftinge svenefftinge Awaiting requested review from svenefftinge

Assignees
No one assigned
Labels
shell issues related to the core shell vscode issues related to VSCode compatibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[debug][ui/ux] Align the Debug View with VS Code
3 participants
@spoenemann @kittaakos @akosyakov