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

tabbar: support icon-less items #12804

Merged
merged 2 commits into from
Aug 31, 2023
Merged

tabbar: support icon-less items #12804

merged 2 commits into from
Aug 31, 2023

Conversation

cdamus
Copy link
Contributor

@cdamus cdamus commented Aug 7, 2023

What it does

VS Code renders action buttons in the tab bar for commands that do not have icons using their "title" text, instead. This commit does the same in Theia.

Additionally, two related minor issues are fixed:

  • the $(icon) specifiers for icons show in the tooltip of an action, which is confusing
  • the roll-over highlight shows only on action buttons for commands that use the "icon" property, not those that embed icon specifiers in their titles

I am happy to extract these fixes into separate PRs if you do not feel it appropriate to join them to the primary issue at hand.

Fixes #12686

How to test

  1. Clone the menu_bug_1 branch of the OP's sample repository as indicated in the issue description.
  2. Build the myext extension in that sample repository and link it into the plugins/ directory of your Theia build.
  3. Launch Theia and open a code editor on some file (e.g., a TS or JSON file from the OP's sample repository).
  4. See the "Goodbye World" action presented in the toolbar next to the pencil-icon "Hello World" action as shown below, and verify that it works (showing a message toast).

CleanShot 2023-08-07 at 08 57 50

Other tests that I would recommend, especially covering the ancillary issues fixed herein, include:

  • append a Font Awesome icon specifier to the title of the "Goodbye World" command and see that it renders the icon and tooltip plausibly like this example using $(cube):
    CleanShot 2023-08-07 at 09 34 13
  • remove the "title" property of the myext.goodbyeWorld command from the myext extension's package.json and see that at least the ID is shown for the command, which will indicate to the developer of this extension that something is amiss (otherwise there would just be a blank spot in the toolbar which could possibly be unobserved)
    CleanShot 2023-08-07 at 09 47 58

Review checklist

Reminder for reviewers

vince-fugnitto
vince-fugnitto reviewed Aug 7, 2023
View reviewed changes
packages/core/src/browser/label-parser.spec.ts Outdated
@@ -32,7 +32,7 @@ before(() => {
statusBarEntryUtility = testContainer.get(LabelParser);
});

describe('StatusBarEntryUtility', () => {
describe.only('StatusBarEntryUtility', () => {
Copy link
Member

Choose a reason for hiding this comment

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

@cdamus please remove the .only or only this test in @theia/core will effectively run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh gosh, that was silly.

Copy link
Contributor Author

@cdamus cdamus Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Commit d17284f fixes that and I've verified that the entire core suite (495 tests run and 2 skipped) is restored.

@cdamus cdamus requested a review from vince-fugnitto August 11, 2023 16:10
@vince-fugnitto vince-fugnitto added vscode issues related to VSCode compatibility ui/ux issues related to user interface / user experience toolbar issues related to the toolbar labels Aug 21, 2023
vince-fugnitto
vince-fugnitto approved these changes Aug 21, 2023
View reviewed changes
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.

The changes work well and look good to me 👍
I'll ask others to review if they have any feedback, if not we can still merge for this month's release.

@vince-fugnitto vince-fugnitto requested a review from msujew August 21, 2023 13:39
@cdamus
Copy link
Contributor Author

cdamus commented Aug 28, 2023

This fix does not interact well with other changes merged in the mean-time to master, so I've some more work to do.

cdamus added 2 commits August 28, 2023 17:19
@cdamus
tabbar: fix command contributions regression
b702ee0 
The merge of eclipse-theia#12799 introduced a regression in
the rendering of MenuToolbarItems that aren't
pop-up menus but instead just commands that
happen to have a menu path: they were rendered
as pop-up menus instead of simple buttons.

This fix refines the condition for the new pop-up
menu rendering to check that the menu to be
rendered is not a command to be executed
directly on click.

Fixes eclipse-theia#12687

Signed-off-by: Christian W. Damus <[email protected]>
@cdamus
tabbar: support icon-less items
a608abe 
VS Code renders action buttons in the tab bar
for commands that do not have icons using
their title text, instead. This commit does the
same in Theia.

Additionally, two related minor issues are fixed:

- the $(icon) specifiers for icons show in the
  tooltip of an action, which is confusing
- the roll-over highlight shows only on action
  buttons for commands that use the "icon"
  property, not those that embed icon specifiers
  in their titles

Fixes eclipse-theia#12686

Signed-off-by: Christian W. Damus <[email protected]>
@cdamus cdamus mentioned this pull request Aug 28, 2023
Closed
@cdamus
Copy link
Contributor Author

cdamus commented Aug 28, 2023

This fix does not interact well with other changes merged in the mean-time to master, so I've some more work to do.

So it wasn't as much a bad interaction of this fix with intervening changes as it was a regression introduced on master by those other changes (specifically #12799). Commit a608abe is rebased onto PR #12869 that I've raised separately to fix that regression in its original context.

martin-fleck-at
martin-fleck-at approved these changes Aug 29, 2023
View reviewed changes
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Thank you for re-working this based on the current master! As far as I can tell everything works now as expected, regular items, run items, etc. Great work!
@vince-fugnitto Do you want to have another look at this?

@martin-fleck-at
Copy link
Contributor

@vince-fugnitto I'm merging this for now. If you encounter any problems with it, please let me know.

@martin-fleck-at martin-fleck-at merged commit 109adc4 into eclipse-theia:master Aug 31, 2023
@cdamus cdamus deleted the issue/12686 branch August 31, 2023 10:37
@vince-fugnitto vince-fugnitto added this to the 1.41.0 milestone Aug 31, 2023
This was referenced Nov 27, 2023
Open
Open
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto approved these changes

@martin-fleck-at martin-fleck-at martin-fleck-at approved these changes

@msujew msujew Awaiting requested review from msujew

Assignees
No one assigned
Labels
toolbar issues related to the toolbar ui/ux issues related to user interface / user experience vscode issues related to VSCode compatibility
Projects
PR Backlog
Archived in project
Milestone
1.41.0
Development

Successfully merging this pull request may close these issues.

Icon-less VS Code “editor/title” menu contribution not rendered properly
3 participants
@cdamus @martin-fleck-at @vince-fugnitto