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.

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

Re-calc menu width when props change. #1014

Merged
merged 3 commits into from
Jul 6, 2015

Conversation

rolandpoulter
Copy link
Contributor

For #805
image

@hai-cea
Copy link
Member

hai-cea commented Jul 2, 2015

@rolandpoulter - It looks like this PR broke the menu open/close transitions.

@rolandpoulter
Copy link
Contributor Author

@hai-cea from my end they appear to be working (Chrome), though they might not behave how they should describe by the design doc.

What is it doing or not doing?

I wouldn't have expected this one to break the transition, my other PR for menus touched that code.

@rolandpoulter
Copy link
Contributor Author

Is it the close transition that isn't working?

@hai-cea
Copy link
Member

hai-cea commented Jul 2, 2015

Yes, the dropdown menu appears and disappears without transitions. I'm using chrome as well.

@rolandpoulter
Copy link
Contributor Author

It seems to work the first time, but stops after opening and closing a few times. I'll see if I can fix it.

@rolandpoulter
Copy link
Contributor Author

@hai-cea I have some questions about the Dom.withoutTransition() method. Why does it set el.style.tranistion to and empty string, instead of caching the previous value and restoring it?

I had the transitions working, but they weren't working smoothly until I took the calls to Dom.withoutTransition out. I believe this function was being called duration a transition and it would cause the transition to be canceled more or less.

Here is an animation of the final result on my machine:
untitled gif 1

@rolandpoulter
Copy link
Contributor Author

The animation doesn't do justice, it actually looks much smoother 😄

@@ -275,7 +280,6 @@ var Menu = React.createClass({
paddingRight: this.context.muiTheme.component.menuSubheader.padding
},
hideable: {
opacity: (this.props.visible) ? 1 : 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Menu would disappear mid animation with this.

@@ -60,6 +60,7 @@ let DropDownMenu = React.createClass({
},

componentWillReceiveProps(nextProps) {
if (this.props.autoWidth) this._setWidth();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding this makes the DropDown button width update as well as the menu width.

@hai-cea
Copy link
Member

hai-cea commented Jul 3, 2015

@rolandpoulter I believe that may be a bug? I think Dom.withoutTransition() should save the current transition so that it can be restored at the end of the style mutation.

@rolandpoulter
Copy link
Contributor Author

I'll look into fixing that on Monday. My worry is that it will effect behaviors throughout the project. Should I work on that as part of this PR or should it be a separate PR?

@hai-cea
Copy link
Member

hai-cea commented Jul 5, 2015

@rolandpoulter Thanks - I think it should be a separate PR.

@rolandpoulter
Copy link
Contributor Author

@hai-cea This method is currently only called in menu.jsx.

hai-cea added a commit that referenced this pull request Jul 6, 2015
Re-calc menu width when props change.
@hai-cea hai-cea merged commit e205ece into mui:master Jul 6, 2015
@hai-cea
Copy link
Member

hai-cea commented Jul 6, 2015

Thanks @rolandpoulter

@zannager zannager added the component: menu This is the name of the generic UI component, not the React module! label Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: menu This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants