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

#8966: Update ViewContainer Toolbars #8967

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Jan 18, 2021

Signed-off-by: Colin Grant [email protected]

What it does

Fixes #8966 by adding code to update a ViewContainer part toolbar when a change event is emitted by the TabbarToolbarRegistry.

How to test

Toolbar Part
  1. Register a toolbar item that includes a didChange event and should appear in a ViewContainer part (for example, one that is attached to part of the debug panel or one that is attached to the file explorer when a plugin contribution (e.g. NPM) is also present).
  2. Hover to reveal the toolbar and fire the command to trigger the didChange event.
  3. Observe that the event does affect the ViewContainer part toolbar.

After the fix

Review checklist

Reminder for reviewers

@colin-grant-work colin-grant-work changed the title [bugfix]: Update ViewContainer Part Toolbars #8967: Update ViewContainer Part Toolbars Jan 18, 2021
@colin-grant-work colin-grant-work changed the title #8967: Update ViewContainer Part Toolbars #8966: Update ViewContainer Part Toolbars Jan 18, 2021
@vince-fugnitto vince-fugnitto added bug bugs found in the application toolbar issues related to the toolbar labels Jan 19, 2021
@colin-grant-work colin-grant-work force-pushed the bugfix/update-part-toolbar branch from 725f1f2 to 8f8170a Compare January 20, 2021 19:41
@colin-grant-work colin-grant-work changed the title #8966: Update ViewContainer Part Toolbars #8966: Update ViewContainer Toolbars Jan 20, 2021
@colin-grant-work colin-grant-work force-pushed the bugfix/update-part-toolbar branch from 8f8170a to 725f1f2 Compare January 20, 2021 19:57
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

Approving only based on the code which LGTM.

@paul-marechal
Copy link
Member

@colin-grant-work is it possible to contribute such a toolbar item in the sample-apis?

@colin-grant-work
Copy link
Contributor Author

Yes, definitely. I can do that to make testing easier 👍

@colin-grant-work colin-grant-work force-pushed the bugfix/update-part-toolbar branch from 725f1f2 to 1187710 Compare April 20, 2021 17:32
@colin-grant-work
Copy link
Contributor Author

colin-grant-work commented Apr 20, 2021

@marechal-p, I've finally gotten around to adding the demo for this to the sample API. I've put a new widget in the explorer area, and if you test with that commit on master, you'll see that the icons don't change until you move your cursor off of the widget, but with the new code from this branch, the icon changes when you click it or when you trigger the action via the button in the widget.

One question adding that widget raises is whether there's a way to use the plugin contributions points within Theia? It seems like there isn't an easy way to say that a view should be added to e.g. the Explorer view container unless it's a plugin.

@colin-grant-work colin-grant-work force-pushed the bugfix/update-part-toolbar branch 2 times, most recently from b715c07 to 58bfb2f Compare April 20, 2021 17:41
@colin-grant-work colin-grant-work force-pushed the bugfix/update-part-toolbar branch from 58bfb2f to 09af5d5 Compare August 18, 2021 23:56
@vince-fugnitto vince-fugnitto self-requested a review August 19, 2021 12:20
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 look good to me, I only have some very minor comments regarding formatting 👍

I confirmed (using the provided sample), that the toolbar item is visible when hovering the part, and that clicking either the toolbar item or the button will update the icon correctly. I also confirmed that on master (rolling back view-container.ts) the hovering does work for the same use-case, but clicking the toolbar item or the button will not update the toolbar.

"@theia/vsx-registry": "1.16.0",
"@theia/workspace": "1.16.0"
"@theia/workspace": "1.16.0",
"@theia/navigator": "1.16.0",
Copy link
Member

Choose a reason for hiding this comment

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

minor: add @theia/navigator in alphabetical order, when we perform a release lerna will change the order anyways.

@@ -24,6 +24,7 @@ import { bindSampleFileWatching } from './file-watching/sample-file-watching-con
import { bindVSXCommand } from './vsx/sample-vsx-command-contribution';

import '../../src/browser/style/branding.css';
import { bindSampleViewContainerPart } from './view/sample-view-container-part-contribution';
Copy link
Member

Choose a reason for hiding this comment

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

minor: add the import with previous imports, we generally add the css import on it's own.

Comment on lines +50 to +51
registry.registerCommand(CHANGE_TO_PLUS_COMMAND,
{
Copy link
Member

Choose a reason for hiding this comment

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

minor: syntax for consistency when registering commands (will auto-format correctly after save):

Suggested change
registry.registerCommand(CHANGE_TO_PLUS_COMMAND,
{
registry.registerCommand(CHANGE_TO_PLUS_COMMAND, {

@colin-grant-work
Copy link
Contributor Author

Closing in favor of #9935, which improves the behavior further.

@colin-grant-work colin-grant-work deleted the bugfix/update-part-toolbar branch April 14, 2022 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application toolbar issues related to the toolbar
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tabbar Toolbar Update Behavior Inconsistent
3 participants