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

Remove mini-browser dependency in plugin-ext #6644

Merged
merged 1 commit into from
Nov 27, 2019

Conversation

thegecko
Copy link
Member

Signed-off-by: Rob Moran rob.moran@arm.com

What it does

This PR removes the direct dependency on mini-browser from plugin-ext.
This means developers can use the plugin-ext extension without having to also accept the mini-browser functionality.

How to test

Clone this branch and remove @theia/mini-browser and @theia/preview from the example application.

Run the application and confirm the mini-browser functionality no longer appears (e.g. f1 > url preview)

Review checklist

Reminder for reviewers

@akosyakov akosyakov added the plug-in system issues related to the plug-in system label Nov 27, 2019
@@ -49,6 +47,8 @@ import { WebviewResourceLoader } from '../../common/webview-protocol';
import { WebviewResourceCache } from './webview-resource-cache';
import { Endpoint } from '@theia/core/lib/browser/endpoint';

const TRANSPARENT_OVERLAY_STYLE = 'theia-mini-browser-transparent-overlay';
Copy link
Member

Choose a reason for hiding this comment

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

ah, but it won't work :( style declaration comes from the mini browser extension

@kittaakos Could we rename the style and move it to the core? It would be generally useful for iframes. We use it already for webview and mini-browser iframes. What would be the good name?

Copy link
Member

Choose a reason for hiding this comment

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

I was about to suggest the same thing, and that it should be a generic style moved to core to avoid the issue. Perhaps the style can be renamed as theia-transparent-overlay (loses the mini-browser in the name)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack, @vince-fugnitto please feel free to share the suggested changes for me to add or open a separate PR.

Alternatively I can switch this PR to a branch on Theia so we can collaborate?

Copy link
Member

Choose a reason for hiding this comment

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

@thegecko sure, all I would do is move this to an appropriate stylesheet in @theia/core:

.theia-mini-browser-transparent-overlay {
background-color: transparent;
position: absolute;
top: 0;
left: 0;
height: 100%;
width: 100%;
z-index: 999;
}

Rename the class theia-mini-browser-transparent-overlay to theia-transparent-overlay, and rename the following as well (to theia-transparent-overlay):

export const TRANSPARENT_OVERLAY = 'theia-mini-browser-transparent-overlay';

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

@akosyakov akosyakov added the mini-browser issues related to the mini-browser label Nov 27, 2019
Signed-off-by: Rob Moran <rob.moran@arm.com>
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.

I verified and the mini-browser is no longer included (when starting the example-browser without preview and mini-browser).

Screen Shot 2019-11-27 at 8 31 20 AM

@thegecko thegecko merged commit 0524a75 into eclipse-theia:master Nov 27, 2019
@thegecko thegecko deleted the remove-plugin-browser branch November 27, 2019 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mini-browser issues related to the mini-browser plug-in system issues related to the plug-in system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants