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

Add 'title' for widgets in the sidebar #5948

Merged
merged 1 commit into from
Aug 15, 2019
Merged

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Aug 14, 2019

What it does

  • added title to widgets present in the sidebar by using the widget's caption.
  • adjusted the explorer to display the workpace uri path when hovering over
    the widget's title.

Examples: (when hovering the title)

Screen Shot 2019-08-14 at 12 27 48 PM

Screen Shot 2019-08-14 at 12 29 50 PM

How to test

  • test with as many widgets as possible.
  • hover over the title for widgets present in the sidebar (a html title should now be displayed)
  • hovering over the explorer title should display the workspace uri path when in a single-root

Review checklist

Reminder for reviewers

Signed-off-by: Vincent Fugnitto vincent.fugnitto@ericsson.com

@vince-fugnitto vince-fugnitto added the enhancement issues that are enhancements to current functionality - nice to haves label Aug 14, 2019
@vince-fugnitto vince-fugnitto self-assigned this Aug 14, 2019
@akosyakov
Copy link
Member

We need to test it with plugin system adding views to the explorer and then toggle between single and multi view modes to check that captions look good in all cases.

@vince-fugnitto
Copy link
Member Author

We need to test it with plugin system adding views to the explorer and then toggle between single and multi view modes to check that captions look good in all cases.

How can I test that case? The same way described in your PR #5665 ?

@akosyakov
Copy link
Member

akosyakov commented Aug 15, 2019

yes, you can adjust the sample extension to contribute to the explorer

The view container goes in the single view mode if only one view is visible, so just hide others. In the multi view mode, the caption should not contain any additional info, since it is not clear from which view is coming from, but the caption of view part for the explorer should have.

Please see how the view part widget title is used to be a title for view part header and view container toolbar in the multi and single view mode correspondingly. The caption should follow the same pattern.

@vince-fugnitto
Copy link
Member Author

I tested using the tree-view-sample with both the single and multi views. The code handles the case where no caption is present by simply displaying the widget's title instead.

Multi-View (Explorer + JSON Outline)

a

Single-View (Explorer Only)

b

@akosyakov
Copy link
Member

@vince-fugnitto What do you see when you hover over TREE-VIEW-SAMPLE in the multi view mode?

@vince-fugnitto
Copy link
Member Author

@vince-fugnitto What do you see when you hover over TREE-VIEW-SAMPLE in the multi view mode?

The panels don't display an individual title, should we add them for those items as well?

c

@akosyakov
Copy link
Member

The panels don't display an individual title, should we add them for those items as well?

I thought it was the issue that @lmcbout wanted to see the path whenever he hovers over the project root as it was before?

@vince-fugnitto
Copy link
Member Author

I thought it was the issue that @lmcbout wanted to see the path whenever he hovers over the project root as it was before?

Yes that's true, apparently I only handle the case of displaying the path when there is a single-view, but I should also do something similar for when it's in a multi-view.

@akosyakov
Copy link
Member

@vince-fugnitto
Copy link
Member Author

Basically caption should be set (and updated) here:

https://github.com/theia-ide/theia/blob/e8a29ca5690548b1a7fa0a646274be2c3e9b0426/packages/navigator/src/browser/navigator-frontend-module.ts#L55-L59

and then everything should work

What will be fixed or work if we set the caption at this point?
I still see that individual views part of the explorer do not have captions for their parent items (ex: TREE-VIEW-SAMPLE or JSON Outline)

@akosyakov
Copy link
Member

@vince-fugnitto you are right, we should set caption on the navigator widget: https://github.com/theia-ide/theia/blob/a878bbfe0085393bac144995788cac8f25a6e64a/packages/core/src/browser/view-container.ts#L211

There is be a bug in the view part that we only display title label ignoring the caption: https://github.com/theia-ide/theia/blob/a878bbfe0085393bac144995788cac8f25a6e64a/packages/core/src/browser/view-container.ts#L846-L850

I think if we should use the caption here as well as a caption for the view part header.

- added `title` to widgets present in the sidebar by using the widget's caption.
- added `caption` title to view containers.
- adjusted the `explorer` to display the workpace uri path when hovering over
the widget's title.

Signed-off-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>
@vince-fugnitto
Copy link
Member Author

I've updated the code to also support captions for view-containers.
Testing with the explorer and debug widgets I can see the following:

d
e

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

@vince-fugnitto I've tested in single root and multi-root workspaces. It works well!

@vince-fugnitto
Copy link
Member Author

@akosyakov should I let it sit for a day? :)

@akosyakov
Copy link
Member

akosyakov commented Aug 15, 2019

@vince-fugnitto we have not agreed on such things and did not document it anywhere, so no for now :) and with current review activity, doubt that somebody else will have a look

@vince-fugnitto
Copy link
Member Author

@vince-fugnitto we have not agreed on such things and did not document it anywhere, so no for now :) and with current review activity, doubt that someone else will have a look

Okay sounds good, thank you for the help and review I appreciate it! :)

@vince-fugnitto vince-fugnitto merged commit 9105c43 into master Aug 15, 2019
@vince-fugnitto vince-fugnitto deleted the vf/workspace-uri branch August 15, 2019 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement issues that are enhancements to current functionality - nice to haves
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants