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

dropdown menu using popover #2150

Merged
merged 1 commit into from
Dec 8, 2015
Merged

dropdown menu using popover #2150

merged 1 commit into from
Dec 8, 2015

Conversation

chrismcv
Copy link
Contributor

This one has 1 issue.

Because the menu-items aren't rendered or on the DOM until the popover is open, the autoWidth can't work the same way. Please have a look and make sure you're happy with how it's behaving before merging. Open to suggestions for fixes.

@chrismcv chrismcv force-pushed the dropdown-popover-menu branch from 1e9b812 to e633295 Compare November 12, 2015 17:33
@chrismcv
Copy link
Contributor Author

@oliviertassinari - you'll be pleased to hear that this is the last PR from #1845!

@oliviertassinari
Copy link
Member

Will fix #1311

@chrismcv chrismcv mentioned this pull request Nov 20, 2015
Closed
oliviertassinari
oliviertassinari reviewed Nov 23, 2015
View reviewed changes
src/drop-down-icon.jsx
@@ -128,8 +128,9 @@ const DropDownIcon = React.createClass({
);
},

_onControlClick() {
_onControlClick(e) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we use event to be more explicit?

@oliviertassinari
Copy link
Member

Looking at the material spec (https://www.google.com/design/spec/components/menus.html#menus-usage) I think that the opening animation is wrong. It should from top to bottom and not from top left to bottom right.

@oliviertassinari oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed Review: review-needed labels Nov 24, 2015
@chrismcv
Copy link
Contributor Author

@oliviertassinari - this one is proving difficult, the animate logic in popover is fairly hardcoded presently, which was based on the iconMenu animations. I'm wondering if we need to pull this out to further components that can manage more custom animations. (e.g. slideDownFromTop). Have you any thoughts or opinions?

@chrismcv
Copy link
Contributor Author

@oliviertassinari thoughts on using https://github.com/twitter-fabric/velocity-react for animations (starting with the popover)?

@oliviertassinari
Copy link
Member

What about https://github.com/chenglou/react-motion ?

@oliviertassinari oliviertassinari mentioned this pull request Nov 30, 2015
Merged
@zachguo
Copy link
Contributor

zachguo commented Dec 4, 2015

I hope this can be merged earlier to fix basic issues such as #2220 #2364 #642

I'm afraid there won't be a quick solution to achieve perfect animation. Also, have a look at the animation for Textfield dropdown in the specs mentioned above. The animation starts from neither top nor top-left, but middle.

@chrismcv
Copy link
Contributor Author

chrismcv commented Dec 4, 2015

@zachguo - i've just created #2367 which should be the first block to this PR. I agree that this is important! I'm going to try and create the dropdown animation now so it is roughly correct

@chrismcv chrismcv force-pushed the dropdown-popover-menu branch from e229770 to 84614f0 Compare December 4, 2015 16:35
@chrismcv
Copy link
Contributor Author

chrismcv commented Dec 4, 2015

@oliviertassinari - this is good to review again

@alitaheri
Copy link
Member

@oliviertassinari Please review this, we need this to close like 20 issues 😄

@oliviertassinari oliviertassinari added Review: review-needed and removed 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 7, 2015
@oliviertassinari
Copy link
Member

Oups, I have forgotten about this one!

@chrismcv
Copy link
Contributor Author

chrismcv commented Dec 7, 2015

I'm adding a maxHeight property at the moment, but continue reviewing - and I'll roll your changes in too.

oliviertassinari
oliviertassinari reviewed Dec 7, 2015
View reviewed changes
src/popover/popover-animate-from-top.jsx
import ThemeManager from '../styles/theme-manager';
import Paper from '../paper';

const PopoverAnimation = React.createClass({
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about PopoverAnimationFromTop?

@chrismcv
Copy link
Contributor Author

chrismcv commented Dec 7, 2015

  1. remove the setTimeout from drop-down-menu
  2. open a dropdown on the docs site
  3. scroll the page so that the drop-down anchor is off screen
  4. everything is locked up

@chrismcv
Copy link
Contributor Author

chrismcv commented Dec 7, 2015

ok - think the setTimeout issue is sorted 😄

@chrismcv chrismcv force-pushed the dropdown-popover-menu branch from 82ceb5d to b918a45 Compare December 7, 2015 20:20
oliviertassinari
oliviertassinari reviewed Dec 7, 2015
View reviewed changes
src/popover/popover.jsx
@@ -58,6 +60,7 @@ const Popover = React.createClass({

getInitialState() {
this.setPlacementThrottled = throttle(this.setPlacement, 100);
this.setPlacementScrolling = throttle(this.setPlacement.bind(this, true), 100);
Copy link
Member

Choose a reason for hiding this comment

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

setPlacementThrottledScrolled?

Copy link
Member

Choose a reason for hiding this comment

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

this broke scrolling. renaming setPlacementScrolling to setPlacementThrottledScrolled fixes it.

@oliviertassinari
Copy link
Member

@chrismcv That's preaty good! We are almost there.

@alitaheri
Copy link
Member

@chrismcv Nice work 👍 👍 👍

@chrismcv chrismcv force-pushed the dropdown-popover-menu branch from b918a45 to 01fe957 Compare December 8, 2015 09:50
@chrismcv
Copy link
Contributor Author

chrismcv commented Dec 8, 2015

@oliviertassinari - those last 2 changes are done - i've squashed as well - anything else you need?

@oliviertassinari
Copy link
Member

@chrismcv Thanks! The source code looks ok 👍.
I can't test it now. @subjectix Do you want to make sure everything work fine and merge?

@alitaheri
Copy link
Member

@oliviertassinari I already did. it works fine.

alitaheri added a commit that referenced this pull request Dec 8, 2015
@alitaheri
Merge pull request #2150 from chrismcv/dropdown-popover-menu
52ed69c 
dropdown menu using popover
@alitaheri alitaheri merged commit 52ed69c into mui:master Dec 8, 2015
@alitaheri
Copy link
Member

Thank you @chrismcv 👍 👍 🎉 🎉

@chrismcv
Copy link
Contributor Author

chrismcv commented Dec 8, 2015

and you guys too!

This was referenced Dec 8, 2015
Closed
Closed
Closed
Closed
This was referenced Dec 8, 2015
Closed
Closed
@rdagger
Copy link

rdagger commented Dec 29, 2015

Do you plan on implementing scrolling for the dropdown?

@oliviertassinari
Copy link
Member

Isn't this already implemented? (checkout the doc)

@rdagger
Copy link

rdagger commented Dec 29, 2015

@oliviertassinari Looks like it only works if you specify a maxHeight which is OK. Thanks.

@chrismcv chrismcv deleted the dropdown-popover-menu branch February 1, 2016 21:12
@zannager zannager added component: menu This is the name of the generic UI component, not the React module! component: Popover The React component. labels Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
component: menu This is the name of the generic UI component, not the React module! component: Popover The React component.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@chrismcv @oliviertassinari @zachguo @alitaheri @rdagger @zannager