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

core: fix displaying for large ConfirmDialog dialogs #12052

Merged

Conversation

jonah-iden
Copy link
Contributor

@jonah-iden jonah-iden commented Jan 9, 2023

Signed-off-by: Jonah Iden [email protected]

What it does

Fixes: #11985.

smaller css fixes for the confirm dialog so that it respects linebreaks and is scrollable in case of too much text.
I used pre-line for automatic line breaks so the dialog only needs vertical scroll though it could be nice if stacktraces or simmilar long lines are supposed to be displayed in it, to also give it horizontal scrolling

How to test

in sample-menu-contributions.ts in the sample command instead of returning an alert we can create a ConfirmDialog.
something like this

dialog = new ConfirmDialog({
    title: 'Sample Dialog',
    msg: 'A\r\nA\r\nA\r\nA\r\nA\r\nA\r\nA\r\nA\r\nA\r\nA\r\nA\r\nA\r\nA\r\nA\r\nA\r\nA\r\nA\r\nA\r\nA\r\nA\r\nA\r\nA\r\nA\r\n'
});
return dialog.open();

clicking on sampleCommand in theia should then open the dialog with scrollable multiline message.

Review checklist

Reminder for reviewers

@paul-marechal
Copy link
Member

I pushed a commit adding a sample command to try your feature and it seems like there is a bug: The text seems to be truncated where if the text is big enough it only displays the 100 last lines or so?

@jonah-iden
Copy link
Contributor Author

oh yeah your totally right. Seems to be because of the display: flex. I'll look into it

@vince-fugnitto vince-fugnitto added dialogs issues related to dialogs ui/ux issues related to user interface / user experience labels Jan 10, 2023
@paul-marechal
Copy link
Member

The changes are looking better, but I noticed that the main area goes down a bit too close to the buttons?

image

Notice how the scrollbar controls eat a bit of the OK button, so does the blue area.

@paul-marechal paul-marechal changed the title linebreaks max-height and scroll for confirmdialog core: fix displaying for large ConfirmDialog dialogs Jan 13, 2023
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 I can confirm that this addresses the overflow and the newline issue.

@msujew msujew merged commit 1b5ff9e into eclipse-theia:master Jan 18, 2023
@vince-fugnitto
Copy link
Member

@jonah-iden @msujew I believe we should revert the changes or fix the regressions since the changes break dialogs like "open folder":

image

@jonah-iden
Copy link
Contributor Author

thats not good at all. Im really sorry for that.
I may have a fix for that though. I'll link it here as soon as i created the PR

jonah-iden added a commit to jonah-iden/theia that referenced this pull request Jan 19, 2023
paul-marechal pushed a commit that referenced this pull request Jan 20, 2023
@paul-marechal paul-marechal added this to the 1.34.0 milestone Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialogs issues related to dialogs ui/ux issues related to user interface / user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dialog: ConfirmDialog should respect line-breaks for msg
4 participants