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

output: improve output display #7827

Merged
merged 1 commit into from
May 26, 2020
Merged

output: improve output display #7827

merged 1 commit into from
May 26, 2020

Conversation

vince-fugnitto
Copy link
Member

What it does

The previous font-weight of the ouput-view made the visibility difficult (especially on lighter themes), and this pull-request updates it to be more consistent and easier to use.

The following pull-request updates the output-view to improve the output display:

  • updates the output-view content (including an update to the font-weight).
  • refactors the output-view to make use of css variables.
  • renames --theia-terminal-font-family to the more generic name --theia-monospace-font-family (the variable was previously unused).
Before & After
_Before_:
Screen Shot 2020-05-15 at 10 07 32 AM Screen Shot 2020-05-15 at 10 07 19 AM

After:

Screen Shot 2020-05-15 at 10 04 10 AM Screen Shot 2020-05-15 at 10 04 16 AM

How to test

master:

  1. start the application, open the output-view and select 'my test channel'
  2. view the output under different themes (light + dark) - the output is difficult to see conformably

pull-request:

  1. perform the same steps above with the pull-request built

Alternatively you may also set the JSON trace to verbose and open a JSON file (ex: 'package.json') to see much more output.

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto vincent.fugnitto@ericsson.com

The following commit performs the following updates:
- improves the display of the output content (output-view).
- refactors the ouput-view to make use of css variables.
- renames the `--theia-termina-font-family` variable.

Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
@vince-fugnitto vince-fugnitto added output issues related to the output ui/ux issues related to user interface / user experience labels May 15, 2020
@vince-fugnitto vince-fugnitto requested a review from lmcbout May 15, 2020 14:22
@vince-fugnitto vince-fugnitto self-assigned this May 15, 2020
@kittaakos
Copy link
Contributor

We will switch to monco editor with #7570. Would that make this issue obsolete?

@vince-fugnitto
Copy link
Member Author

vince-fugnitto commented May 15, 2020

We will switch to monaco editor with #7570. Would that make this issue obsolete?

Yes I remember :) The switch to the monaco-editor will make the output-view will definitely improve the view (like vscode).

I noticed the output was not easy to read during a pull-request review and thought that a quick fix (even if obsolete when the switch happens) might be an improvement to end-users. I also do not know when the switch will happen as the pull-request is already quite involving and still needs to be finalized and reviewed.

If you do not believe the effort is worth it, I can close the pull-request without any problems :)

@kittaakos
Copy link
Contributor

make this issue obsolete?

can close the pull-request

No no, I was stupid and did not notice it is already a PR. It affects only the electron-based app, right? I am happy to verify right now...

@vince-fugnitto
Copy link
Member Author

make this issue obsolete?

can close the pull-request

No no, I was stupid and did not notice it is already a PR. It affects only the electron-based app, right? I am happy to verify right now...

No problem! It is for both the browser and electron targets.

@kittaakos
Copy link
Contributor

kittaakos commented May 15, 2020

It is for both the browser

I see.

Browser before:
Screen Shot 2020-05-15 at 17 29 26

Browser after:
Screen Shot 2020-05-15 at 17 29 18

It is a bit too "heavy" for me in the browser...

and electron targets.

but I agree, it is hardly visible in electron (with the dark theme) so it is a reasonable change. 👍 Other committers can vote or decide I do not have any objections. Please keep in mind #7570.


Regarding the code change you did just to understand, it's all equivalent with adding fontWeight: 600 to the object, right?

@kittaakos kittaakos self-requested a review May 15, 2020 15:45
@vince-fugnitto
Copy link
Member Author

Regarding the code change you did just to understand, it's all equivalent with adding fontWeight: 600 to the object, right?

Yes, I refactored the code from the widget itself into the css stylesheet, and updated the font-weight for the monospace font-family. Originally, the font-weight on electron was too thin and made it difficult to easily view the content. I think it will look much better like you said when we start using monaco in the view.

@vince-fugnitto vince-fugnitto merged commit ed676f7 into master May 26, 2020
@vince-fugnitto vince-fugnitto deleted the vf/output-ui branch May 26, 2020 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
output issues related to the output ui/ux issues related to user interface / user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants