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

[transitions] Allow to run Slide into a custom container #26623

Merged
merged 35 commits into from Aug 13, 2021
Merged

[transitions] Allow to run Slide into a custom container #26623

merged 35 commits into from Aug 13, 2021

Conversation

benny0642
Copy link
Contributor

@benny0642 benny0642 commented Jun 6, 2021 •
edited by oliviertassinari

Closes #20250

Preview: https://deploy-preview-26623--material-ui.netlify.app/components/transitions/#slide-relative-to-a-container

change summary

  • Add targetRef prop to Slide component, so that the children of Slide could chose sliding not only from the edge of screen, but also from the edge of targetRef DOM element.
  • Add example into SimpleSlide
  • Add api docs and related translation about the targetRef

In the video

  • The left one is the original slide behavior, sliding from the edge of screen.
  • The right one is passing targetRef (the yellow Box) to the Slide, so the icon could slide from the edge of yellow Box.
  • By the way, the yellow Box is just for explanation. The style would not be committed into this PR.
slide_example04.mp4

This PR is an enhancement about the issue #20250.

A reference closed PR about the issue: #20266

@mui-pr-bot
Copy link

mui-pr-bot commented Jun 6, 2021 •
edited

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against 7a7c0e4

@benny0642 benny0642 changed the title 20250 add target ref to slide component [Transition] Add targetRef prop to Slide comonent Jun 6, 2021
@oliviertassinari oliviertassinari added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jun 6, 2021
@benny0642
Copy link
Contributor Author

@oliviertassinari Excuse me, would any maintainer review this PR? If the maintainers are busy, that's ok, I can wait.

If the maintainers need more information about the PR, please let's me know. Thanks.

@benny0642
Copy link
Contributor Author

benny0642 commented Jun 17, 2021 •
edited

@oliviertassinari I am not sure why this PR haven't been reviewed. Is the description not clear enough? Or is the PR too large to review?

This enhancement would really improve the UI experience for my company's product. I believe there would be someone could get the benefit from it.

So, if maintainers need more information about this PR, please let me know.

Thanks.

@oliviertassinari oliviertassinari deleted the branch mui:next June 22, 2021 00:28
@oliviertassinari oliviertassinari changed the base branch from foo to next June 22, 2021 00:47
mnajdova
mnajdova reviewed Jun 30, 2021
View reviewed changes
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Thanks for working on this and sorry for the delayed review. Could you take a look on the provided comments?

docs/src/pages/components/transitions/SimpleSlide.js Outdated Show resolved Hide resolved
packages/material-ui/src/Slide/Slide.d.ts Outdated Show resolved Hide resolved
packages/material-ui/src/Slide/Slide.js Outdated Show resolved Hide resolved
@benny0642 benny0642 requested a review from mnajdova July 1, 2021 13:15
@github-actions github-actions bot mentioned this pull request Jul 1, 2021
Closed
@benny0642
Copy link
Contributor Author

@mnajdova I updated this PR, so you can review this PR, thanks.

@eps1lon eps1lon added component: transitions This is the name of the generic UI component, not the React module! new feature New feature or request and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jul 13, 2021
michaldudak
michaldudak reviewed Jul 23, 2021
View reviewed changes
Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

I found a few minor things to correct. Aside from that, I like it 👍.

docs/src/pages/components/transitions/transitions.md Outdated Show resolved Hide resolved
packages/material-ui/src/Slide/Slide.d.ts Outdated Show resolved Hide resolved
packages/material-ui/src/Slide/Slide.js Outdated Show resolved Hide resolved
mnajdova
mnajdova approved these changes Jul 23, 2021
View reviewed changes
Copy link
Member

@mnajdova mnajdova 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 pushed few commits, looks good. Thanks very much for the contribution and patience @benny0642 :)

@benny0642
Copy link
Contributor Author

@mnajdova Thanks for updating the docs and cleaning up my codes. It seams that nothing have to be modified. What is the next step?

It's my first time to contribute to the community. Thanks again. 😄

@oliviertassinari oliviertassinari changed the title [Transition] Add targetRef prop to Slide comonent [transitions] Add targetRef prop to Slide comonent Jul 25, 2021
oliviertassinari
oliviertassinari previously requested changes Jul 25, 2021
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Could we add one simple test case as a safety net?

packages/material-ui/src/Slide/Slide.js Outdated Show resolved Hide resolved
packages/material-ui/src/Slide/Slide.js Outdated Show resolved Hide resolved
@benny0642 benny0642 force-pushed the 20250_add_targetRef_to_slide_component branch from 0c13ff4 to 86b32b7 Compare August 8, 2021 12:59
@oliviertassinari oliviertassinari changed the title [transitions] Add targetRef prop to Slide comonent [transitions] Allow to run Slide into a custom container Aug 8, 2021
@oliviertassinari oliviertassinari force-pushed the 20250_add_targetRef_to_slide_component branch from 27f30e5 to 243056e Compare August 8, 2021 14:49
@oliviertassinari
Copy link
Member

I have rebased on HEAD

@oliviertassinari oliviertassinari force-pushed the 20250_add_targetRef_to_slide_component branch 3 times, most recently from 40cb6a9 to 62eb2a5 Compare August 8, 2021 16:17
@oliviertassinari oliviertassinari force-pushed the 20250_add_targetRef_to_slide_component branch from 62eb2a5 to 4854ee6 Compare August 8, 2021 16:34
michaldudak
michaldudak approved these changes Aug 9, 2021
View reviewed changes
Copy link
Member

@michaldudak michaldudak 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!

@oliviertassinari oliviertassinari merged commit 611747b into mui:next Aug 13, 2021
@oliviertassinari
Copy link
Member

@benny0642 Thanks!

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

@oliviertassinari oliviertassinari oliviertassinari left review comments

@michaldudak michaldudak michaldudak approved these changes

@mnajdova mnajdova Awaiting requested review from mnajdova

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

Successfully merging this pull request may close these issues.

[Transition] Add 'parent/anchor' prop to Slide component.
6 participants
@benny0642 @mui-pr-bot @oliviertassinari @mnajdova @michaldudak @eps1lon