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

[mini-browser] Make the mini-browser background white again #4096

Merged
merged 1 commit into from
Jan 21, 2019

Conversation

jankeromnes
Copy link
Member

@jankeromnes jankeromnes commented Jan 17, 2019

Fix #3969 by making the background white again, which is what every web app expects.

(Part of ongoing mini-browser improvements, see #4102.)

@jankeromnes
Copy link
Member Author

@svenefftinge or @akosyakov, I can't reproduce the live-preview flickering issue that you mentioned.

I tried running mdBook in Gitpod, but:

  • The server that starts running by default doesn't refresh the mini-browser (and refreshing manually doesn't flicker even with white background)
  • When I click on the Eye icon next to a markdown file, it opens a Gitpod markdown live-preview that's already designed with white text on dark background (so maybe no need to make it white, and also it doesn't flicker either)

@svenefftinge
Copy link
Contributor

I remember that the reasons we limited the features was that is quite limiting what works in an iframe. So we didn't allow to change the URL (it is just a property that can be set). Also the history is using the outer windows history, so going back will eventually leave the IDE all together.

The mdbook hot code replacing only works after this change (it exposes the websocket): https://gitpod.io/#https://github.com/svenefftinge/mdBook

@svenefftinge
Copy link
Contributor

I think it would make sense to limit this PR to only changing the background. And work on the other parts independently.

@jankeromnes
Copy link
Member Author

I think it would make sense to limit this PR to only changing the background. And work on the other parts independently.

Agreed! I'll move the checklist into an issue, and reduce the scope of this PR to the first item.

I remember that the reasons we limited the features was that is quite limiting what works in an iframe. So we didn't allow to change the URL

This doesn't make sense to me. The URL looks editable, but isn't. Also, using F1 > Open URL you can still open any URL in the Preview pane, so non-editable just makes the UI more frustrating.

But maybe this is linked to another bug, that causes the loading screen to hide the iframe whenever there is an error? (E.g. instead of seeing clear "CSP" or "Content-Encoding" error pages, I still see the "loading" overlay in Gitpod, but deleting that overlay manually using devtools reveals the actual iframe error).

I think if we fix the overlay and clearly show the iframe errors, we can make the URLbar editable again (users will probably understand that if you try to open www.google.com there, it won't work, but it's ok because that's not the point of the Preview pane).

@jankeromnes jankeromnes changed the title [wip][mini-browser] Improve the mini-browser [mini-browser] Make the mini-browser background white again. Jan 18, 2019
@jankeromnes jankeromnes changed the title [mini-browser] Make the mini-browser background white again. [mini-browser] Make the mini-browser background white again Jan 18, 2019
Fixes eclipse-theia#3969

Signed-off-by: Jan Keromnes <jan.keromnes@typefox.io>
@jankeromnes
Copy link
Member Author

jankeromnes commented Jan 21, 2019

Thanks for the review! Rebased + signed off.

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.

None yet

3 participants