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

[Dialog] Add a PaperComponent property & draggable demo #13879

Merged
merged 5 commits into from
Dec 13, 2018
Merged

[Dialog] Add a PaperComponent property & draggable demo #13879

merged 5 commits into from
Dec 13, 2018

Conversation

rfbotto
Copy link
Contributor

@rfbotto rfbotto commented Dec 11, 2018

Closes #9775

oliviertassinari
oliviertassinari requested changes Dec 11, 2018
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment •
edited
Loading

Choose a reason for hiding this comment

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

While I appreciate the effort in making this new demo. I don't think that we can merge as is.
It's a hack. I propose the following, we introduce a PaperComponent property. It works much better.
I have made a POC around it, it should be good, just to give you an idea (quick and dirty):

@oliviertassinari oliviertassinari added new feature New feature or request component: dialog This is the name of the generic UI component, not the React module! labels Dec 11, 2018
@oliviertassinari oliviertassinari changed the title [Dialog] Add react-draggable demo [Dialog] Add a PaperComponent property & draggable demo Dec 11, 2018
@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Dec 11, 2018
@oliviertassinari
Copy link
Member

What do you think about it?

@oliviertassinari oliviertassinari self-assigned this Dec 12, 2018
@oliviertassinari
Copy link
Member

I'm continuing the pull request :)

@rfbotto
Copy link
Contributor Author

rfbotto commented Dec 12, 2018

Hey @oliviertassinari, i like your idea, it allows us to extend the current behaviour without losing any functionality and by also making a cleaner approach to make the draggable work together with the wrapper. I would be happy to finish the PR, just not sure i will have the time today.

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 12, 2018 •
edited
Loading

@rfbotto There are some other changes I want to do. You can review afterward :).

@oliviertassinari oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Dec 12, 2018
@rfbotto
Copy link
Contributor Author

rfbotto commented Dec 12, 2018

Okay, thank you @oliviertassinari :)

@oliviertassinari oliviertassinari removed their assignment Dec 12, 2018
@rfbotto
Copy link
Contributor Author

rfbotto commented Dec 13, 2018

Looks good @oliviertassinari 👍

@oliviertassinari oliviertassinari merged commit fb2122b into mui:master Dec 13, 2018
@oliviertassinari
Copy link
Member

@rfbotto Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari approved these changes

@mbrookes mbrookes mbrookes approved these changes

Assignees
No one assigned
Labels
component: dialog This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation new feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rfbotto @oliviertassinari @mbrookes