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

browser: Modal Dialog fixed display issues #10245

Merged
merged 1 commit into from
Oct 18, 2021

Conversation

Archie27376
Copy link
Contributor

What it does

fixes #8090
Changes made:

  • The close button to read Cancel instead to be similar to vscode
  • Color of Cancel button switched to the accent color of the modal dialog
  • Eliminated the hard-coded Theia title to display the application's name instead

How to test

  • Download the necessary plugin and place in /plugins
  • Start the browser through command line yarn browser start
  • F1 to access the command palette and type Dialog: Open Modal

Review checklist

Reminder for reviewers

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.

Hi @Archie27376, thank you for taking care of this issue!

I think the original issue is a bit outdated, since the styling of the close button seems already correct, see the following comparison with vscode (Browser):

image

Theia, current:

image

Instead, I'm a bit more worried about the missing detail line detail content (see vscode image) and the size and color of the info icon.

Do you mind reverting the current changes to the dialog.css and addressing the detail line and icon issue?

For the icon colors, you can take inspiration from another style sheet:

.theia-marker-container .error {
color: var(--theia-editorError-foreground);
opacity: 1;
}
.theia-marker-container .warning {
color: var(--theia-editorWarning-foreground);
opacity: 1;
}
.theia-marker-container .information {
color: var(--theia-editorInfo-foreground);
opacity: 1;
}
.theia-marker-container .hint {
color: var(--theia-successBackground);
opacity: 1;
}

@msujew msujew added dialogs issues related to dialogs vscode issues related to VSCode compatibility labels Oct 8, 2021
@Archie27376 Archie27376 force-pushed the gh8090 branch 3 times, most recently from 9752d2b to 0e98755 Compare October 13, 2021 14:01
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.

Thanks, that looks way better already. I have one remark left regarding the detail html node.

packages/core/src/browser/style/dialog.css Outdated Show resolved Hide resolved
@Archie27376 Archie27376 force-pushed the gh8090 branch 4 times, most recently from f6f5d4d to e1c7eb5 Compare October 13, 2021 20:01
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.

Almost done, just two small adjustments:

  1. Please set the margin of the p node inside of the .detail class to have this value (to decrease the amount of vertical white space under the detail node): calc(var(--theia-ui-padding) * 2) 0px 0px 0px;
  2. Move the Changes from the dialog.css files to the modal-notification.css and change the .icon .fa rule to .icon .codicon. That should remove the need for all the .codicon rules that you added in the dialog.css file. (but please increase the font-size, 120% seems too small)

@Archie27376 Archie27376 force-pushed the gh8090 branch 3 times, most recently from 328002a to 1c63cd0 Compare October 14, 2021 15:39
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.

Thanks, looks great!

@msujew msujew merged commit cf5ef23 into eclipse-theia:master Oct 18, 2021
@vince-fugnitto vince-fugnitto deleted the gh8090 branch October 18, 2021 15:07
@vince-fugnitto vince-fugnitto added this to the 1.19.0 milestone Oct 19, 2021
msujew added a commit that referenced this pull request Nov 9, 2021
Fixes a bug introduced by #10245 which prevented plugin notifications from being shown
CareyJWilliams pushed a commit to ARMmbed/theia that referenced this pull request Jan 25, 2022
…10399)

Fixes a bug introduced by eclipse-theia#10245 which prevented plugin notifications from being shown
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialogs issues related to dialogs vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[plugin-ext]: Modal dialog enhancements
3 participants