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

React dialog type #6855

Merged
merged 1 commit into from
Feb 6, 2020
Merged

React dialog type #6855

merged 1 commit into from
Feb 6, 2020

Conversation

sfrylmark
Copy link
Contributor

@sfrylmark sfrylmark commented Jan 9, 2020 •
edited
Loading

What it does

Introduces an abstract class that extends AbstractDialog for making dialogs with React, similar to how ReactWidget extends BaseWidget.

How to test

Try Help -> About. That dialog now uses react, make sure it works :)

Review checklist

Reminder for reviewers

vince-fugnitto
vince-fugnitto reviewed Jan 9, 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.

Thank you for your first contribution! 🍾
I noticed a couple of small things, we may also require some CHANGELOG notes about some breaking changes (moving of components).

I tested the AboutDialog and it worked correctly 👍

packages/core/src/browser/about-dialog.tsx Show resolved Hide resolved
packages/core/src/browser/dialogs/abstract-dialog.ts Outdated Show resolved Hide resolved
packages/core/src/browser/dialogs/react-dialog.tsx Show resolved Hide resolved
@vince-fugnitto vince-fugnitto added core issues related to the core of the application dialogs issues related to dialogs labels Jan 9, 2020
@akosyakov
Copy link
Member

If code was moved and then changed, the original copyright has to be preserved without changes.

@sfrylmark
Copy link
Contributor Author

Thanks for the review, guys! I'll resolve the concerns by monday, have a good weekend :)

@sfrylmark
Copy link
Contributor Author

Build failed because of a rate limit or something, should be retried.

@akosyakov
Copy link
Member

I'm fine with refactoring.

@marcdumais-work
Copy link
Contributor

marcdumais-work commented Jan 14, 2020 •
edited
Loading

Build failed because of a rate limit or something, should be retried.

Yes, it's a known limitation in our current CI setup and does not mean there is a problem with your code.

PRs that originate from a fork are not covered by our configured GitHub token, and so the GH "API rate limit" sometimes / often kicks-in when downloading vscode-ripgrep. I'll restart CI and hopefully it will pass this time.

update: seems someone beat me to it - CI is successful ATM.

@sfrylmark
Copy link
Contributor Author

Okay, it's ready for another look 👍

@akosyakov
Copy link
Member

@vince-fugnitto it looks fine to me now

@marcdumais-work
Copy link
Contributor

@efrlsml Thanks for the refactoring.

Please update the commit message to something more descriptive. What you have in the "What it does" section of the description is a good start, but I'd like to see the name of the new dialog class as well.

@sfrylmark sfrylmark force-pushed the react-dialog branch 2 times, most recently from 0b2565a to d954b6a Compare January 30, 2020 15:35
@sfrylmark
Copy link
Contributor Author

Have updated the message and rebased now, thanks @marcdumais-work :)

vince-fugnitto
vince-fugnitto approved these changes Jan 30, 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.

Thank you for your contribution @efrlsml 👍

I code looks fine and the about dialog correctly works as before.
I have a minor comment regarding the changelog then the pull-request can be merged :)

CHANGELOG.md Outdated Show resolved Hide resolved
vince-fugnitto
vince-fugnitto approved these changes Feb 3, 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.

I'm happy with the latest updates :)

@sfrylmark
[core] add a new React-based dialog type ReactDialog
57c7eae 
This makes it easy to make react-based dialogs, by introducing an abstract class called
`ReactDialog` that extends `AbstractDialog` for making dialogs with React, similar to how
`ReactWidget` extends `BaseWidget`.

Signed-off-by: Samuel Frylmark <samuel.frylmark@ericsson.com>
vince-fugnitto
vince-fugnitto approved these changes Feb 6, 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.

I've verified the changes once more, thank your for your contribution @efrlsml ! 👍

@vince-fugnitto vince-fugnitto merged commit 5034fbe into eclipse-theia:master Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akosyakov akosyakov akosyakov left review comments

@westbury westbury westbury left review comments

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

Assignees
No one assigned
Labels
core issues related to the core of the application dialogs issues related to dialogs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sfrylmark @akosyakov @marcdumais-work @westbury @vince-fugnitto