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.

Sign up for GitHub

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

Jump to bottom

[dialogs] Improve display of dialogs #7080

Merged
merged 1 commit into from
Feb 26, 2020
Merged

[dialogs] Improve display of dialogs #7080

merged 1 commit into from
Feb 26, 2020

Conversation

kaiyue0329
Copy link
Contributor

@kaiyue0329 kaiyue0329 commented Feb 4, 2020 •
edited
Loading

What it does

Solves: #6905

  • Allow developers to control the desired width of the dialog

How to test

  • Register a new command in an extension to trigger the opening a dialog with a very long text
  • Call the dialog on execute

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added dialogs issues related to dialogs ui/ux issues related to user interface / user experience labels Feb 4, 2020
vince-fugnitto
vince-fugnitto reviewed Feb 4, 2020
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@kaiyue0329 please provide how to test section so that reviewers can easily test and validate the feature. Please also address the comments present.

packages/core/src/browser/dialogs.ts Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs.ts Outdated Show resolved Hide resolved
vince-fugnitto
vince-fugnitto reviewed Feb 5, 2020
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

There seems to be a CI issue regarding the jsdocs formatting.
Please see: https://travis-ci.com/eclipse-theia/theia/jobs/283842792#L855-L866

@akosyakov
Copy link
Member

@vince-fugnitto looks good to me, could you check that it does not introduce any regressions for existing dialogs please before merging

@vince-fugnitto
Copy link
Member

@kaiyue0329 I tried to update an existing dialog with the new props (maxWidth) and it did not seem to work, were you successful?

Improve display of dialogs
326d54f 
- Allow developers to control the desired width and word-wrap of the dialog
- Add detail documentation for maxWidth and wordWrap props
- Add guard for undefined maxWidth and wordWrap
- Set 'min-width' for the dialog to 0px so that 'max-width' can be applied properly

Signed-off-by: Kaiyue Pan <kaiyue.pan@ericsson.com>
vince-fugnitto
vince-fugnitto approved these changes Feb 11, 2020
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes work correctly, I verified by updating an existing dialog (AboutDialog) and seeing if the new props correctly update the styling of the dialog.

I also verified that existing dialogs are not affected by this pull-request.

@vince-fugnitto vince-fugnitto merged commit 22ad835 into eclipse-theia:master Feb 26, 2020
@vince-fugnitto vince-fugnitto mentioned this pull request Mar 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto approved these changes

@akosyakov akosyakov Awaiting requested review from akosyakov

Assignees
No one assigned
Labels
dialogs issues related to dialogs ui/ux issues related to user interface / user experience
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kaiyue0329 @akosyakov @vince-fugnitto