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

[tabbar][enhancement] Add icons and captions to view tabs #2867

Merged
merged 1 commit into from
Sep 27, 2018

Conversation

gonzalezw
Copy link
Contributor

Changes

  • Adds tab icons to all current views
  • Adds captions to all current views
  • Adds unique classname to all tab icons for easy customization

Fixes Theia issue 716.

Default view:
screen shot 2018-09-12 at 17 13 32

Overriding CSS to display only icons on sides:

.p-TabBar.theia-app-sides .p-TabBar-tabLabel {
    display: none;
}
.p-TabBar.theia-app-sides .p-TabBar-tabIcon {
    display: inline-block;
}

screen shot 2018-09-12 at 17 14 06

Overriding an icon:

.debug-tab-icon::before {
    content: "\f085";
}

screen shot 2018-09-12 at 17 31 25

@vince-fugnitto
Copy link
Member

I do like the idea of having icons represent our different widgets, maybe they can be controlled through a general preference which handles either displaying Labels or Icons.

One comment I do have however is the formatting and alignment of the icons. Based on the screenshot and testing it myself the alignment is off especially when hovering over them.

@akosyakov
Copy link
Member

akosyakov commented Sep 13, 2018

It is not possible to distinguish individual files and icons not aligned:
icons

@kittaakos
Copy link
Contributor

Related: #2865.

@thegecko
Copy link
Member

Icon alignment may need to be undertaken by editing the icons themselves if they don't have consistent borders. I would recommend icon changes are out of scope and should be visited in a separate PR.
This change then offers an option of overriding the icons with custom ones and the default view remains unchanged when not overridden.

Exposing the icons or text choice as a user preference is a good suggestion. However, this could also be added in a further PR in the interest of continuous improvement.

@@ -5,6 +5,10 @@
"dependencies": {
"@theia/core": "^0.3.14",
"@theia/editor": "^0.3.14",
"@theia/workspace": "^0.3.14",
Copy link
Member

Choose a reason for hiding this comment

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

FYI, These additions may clash with the workspace search PR #2648

@vitaliy-guliy
Copy link
Contributor

Why did you give up the idea of using this.title.iconClass = 'fa fa-icon-class-name' and described a custom class for each icon? It's not a problem and not critical to me, I'm just wondering.

@gonzalezw
Copy link
Contributor Author

@vitaliy-guliy, by using the custom class, the icon can be easily customised.

Using fa fa-icon-class-name would required you to overwrite the fa-icon-class-name class if you don't want to use that icon and doesn't provide an easy way to selecting a particular tab icon in CSS.

Whereas fa tab-icon-class still keeps the font-awesome base styling which basically just sets the font family. Then with tab-icon-class we can still set a font-awesome icon using CSS with icon unicode:

.tab-icon-class::before {
    content: "\F058";
}

And if you don't want a font-awesome icon you can clear the icon and replace it with your own. An svg in this case:

.tab-icon-class::before {
    content: none;
}
.tab-icon-class {
    -webkit-mask: url(../icons/files.svg) no-repeat center;
    mask: url(../icons/files.svg) no-repeat center;
}

@svenefftinge
Copy link
Contributor

I'm fine with doing refinements in subsequential PRs, as long as the changes are not visible by default.

@vitaliy-guliy
Copy link
Contributor

@gonzalezw Thanks for clarifying!

I'm implementing Views plugin API like in VS Code. View container in the API looks like parts/views in Theia and has id, title and icon.

So, I'm interested in those improvements and think about following

  • how can I easily use custom icon for a part
  • ensure that custom icon will not broke the Theia UI
  • how can I change icon size and properly display it on the left, right and bottom

@spoenemann spoenemann removed their request for review September 13, 2018 14:56
@gonzalezw
Copy link
Contributor Author

@vitaliy-guliy, how do plugins set their own styling? Perhaps when you build the plugin view, you can also apply the plugin id as a class name? Then this can be used to customise the icon like in the native packages.

The core package sets the default CSS for the side panel tab icons in sidepanel.css.

You can modify the styling using the class p-TabBar-tabIcon and then specify the location:

  • both sides: .p-TabBar.theia-app-sides .p-TabBar-tabIcon
  • left side : .p-TabBar.theia-app-left .p-TabBar-tabIcon
  • right side: .p-TabBar.theia-app-right .p-TabBar-tabIcon

Maybe the class selector .p-TabBar.theia-app-sides .p-TabBar-tabIcon should define some more constraints to keep icons uniform on the sides.

@vitaliy-guliy
Copy link
Contributor

@gonzalezw Thanks. For now plugin does not change Theia styles and UI except what I did in #2810

I have some feedbacks to this PR.

  • highlighting the active icon and changing the font color may look better than playing with background
  • icons are still not properly aligned on both panels
  • how the user can change style of the panel?

@vitaliy-guliy
Copy link
Contributor

Changing the icon opacity for active tab and when hovering would be simpler.
E.g. for default state we can set 60%, for hover and active 100%.
The same VS Code does.

Signed-off-by: William Gonzalez <William.Gonzalez@arm.com>
@gonzalezw
Copy link
Contributor Author

@svenefftinge, this PR keeps the Thea UI untouched. All it really does is add the icons to the Theia widgets that don't have them and captions. And it exposes an easy CSS class to override those icons. But the UI maintains the default side panel look with only text and no icons. From a user perspective, the only difference will be the on hover captions that will now be available.

For developers that are wanting to use the icons on side panels, then they would have to override the default css. For non-Theia widgets (plugins/extensions), it would also be up to them to configure and style with an icon if they want to expose them.

@thegecko
Copy link
Member

What outstanding changes need to be implemented before this can be merged?

@gonzalezw
Copy link
Contributor Author

@thegecko, there are no required changes to keep the existing functionality. There were comments about alignment but I don't see any issues with the default configuration with text only or if viewing by icons only.

@thegecko
Copy link
Member

@svenefftinge happy to approve?

Copy link
Contributor

@svenefftinge svenefftinge left a comment

Choose a reason for hiding this comment

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

LGTM

@thegecko thegecko merged commit fd9dfca into eclipse-theia:master Sep 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants