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

Fix modal dialog max-width and text wrapping #8080

Merged
merged 1 commit into from
Jun 29, 2020

Conversation

tetchel
Copy link
Contributor

@tetchel tetchel commented Jun 23, 2020

Signed-off-by: Tim Etchells timetchells@ibm.com

What it does

Fixes #8071

  • use p instead of pre so that the text will wrap to fit in the modal, but add white-space: pre-wrap so that newlines are still preserved and spaces are not collapsed
  • adds max-width and max-height to modal

How to test

Install the vsix from https://github.com/tetchel/theia-modal-wrap and run the command it contributes

Review checklist

Reminder for reviewers

Screenshot:
image

@tetchel
Copy link
Contributor Author

tetchel commented Jun 23, 2020

@vince-fugnitto PTAL

@akosyakov akosyakov added the vscode issues related to VSCode compatibility label Jun 24, 2020
@kittaakos
Copy link
Contributor

I do not know if this is out of context or not, but the button color does not look good. Please compare the New File, Delete`, and the modal notification dialog.

Screen Shot 2020-06-25 at 15 16 50
Screen Shot 2020-06-25 at 15 17 01
Screen Shot 2020-06-25 at 15 16 43

@kittaakos
Copy link
Contributor

The Close button should be Close and not close. I am going to open a follow-up...

@tetchel
Copy link
Contributor Author

tetchel commented Jun 25, 2020

@kittaakos I agree but didn't want to feature-creep my first PR.

In VS Code, it's Cancel instead of close. I prefer Cancel for consistency so I don't need this (for this message) but it's also a little frustrating sometimes that the cancel button has to be there because there isn't always a "cancellation" action out of every modal. Whereas there is always a close button anyway (the x in the top-right).

@tetchel
Copy link
Contributor Author

tetchel commented Jun 25, 2020

Let me see about the button colour.

@kittaakos
Copy link
Contributor

I agree but didn't want to feature-creep my first PR.

You do not have to, what you've done is pretty slick so far. 👍 I found another issue, so I will create a follow-up anyway: the Command Palette should be disabled when there is a model dialog.

But regarding this PR, I would propose one single change, you set a max-height, and when the model content is really really long, then it looks ugly:

Screen Shot 2020-06-25 at 15 40 21

I propose the following change:

diff --git a/packages/plugin-ext/src/main/browser/dialogs/style/modal-notification.css b/packages/plugin-ext/src/main/browser/dialogs/style/modal-notification.css
index 4cbfa2390..619dcfda2 100644
--- a/packages/plugin-ext/src/main/browser/dialogs/style/modal-notification.css
+++ b/packages/plugin-ext/src/main/browser/dialogs/style/modal-notification.css
@@ -29,7 +29,6 @@
     min-width: 200px;
     max-width: min(66vw, 800px);
     min-height: 35px;
-    max-height: min(66vh, 600px);
     background-color: var(--theia-editorWidget-background);
     margin-bottom: 1px;
     color: var(--theia-editorWidget-foreground);
@@ -67,7 +66,7 @@
     user-select: text;
     align-self: center;
     flex: 1 100%;
-    height: 100%;
+    max-height: min(66vh, 600px);
     padding: 10px;
     overflow: auto;
     white-space: pre-wrap;

And then you can scroll the content:
screencast 2020-06-25 15-39-31

I am not a CSS wizard; what do you think?

@tetchel
Copy link
Contributor Author

tetchel commented Jun 25, 2020

for sure, i missed that height: 100%

Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

I added two minor notes, just to make the changeset cleaner. Also, please squash your commits; this PR should consist of one single commit. Thank you!

Signed-off-by: Tim Etchells <timetchells@ibm.com>
Copy link
Contributor

@kittaakos kittaakos 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, @tetchel! It's great that you have enhanced the test extension with the super long message example 👍

@kittaakos kittaakos merged commit a62011a into eclipse-theia:master Jun 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modal notification does not render newlines or have a max-width
3 participants