Skip to content

Conversation

chrismcv
Copy link
Contributor

This PR created a wrapped dialog component that uses the render to layer component to put the dialog in a separate dom tree.

The reason for the wrapping is so that refs can continue to be used as normal within the dialog component for animation.

(This doesn't depend on the #2043 branch)

src/overlay.jsx Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove this property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

carelessness - merging from #1845 by hand... apologies

@chrismcv
Copy link
Contributor Author

@oliviertassinari - rebasing this wasn't as the warning have moved completely, so git merge had a bad day at the office. I've manually rolled in your changes. Can I suggest no further changes to dialog.jsx get added to master until this is merged (assuming that is planned for soon.)

@oliviertassinari
Copy link
Member

@chrismcv Sorry about this. I had to merge the fix #2142. Otherwise, I agree, let's freeze dialog.jsx.

src/overlay.jsx Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the show property be required instead of adding a default value?

src/overlay.jsx Outdated
Copy link
Member

Choose a reason for hiding this comment

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

There is two setState call. Do you if this will have an impact on perf?
Actually, it looks like we don't need to have show in the state.
Can't we just use the show property?

@chrismcv
Copy link
Contributor Author

@oliviertassinari - your'e right, state not required, have removed :)

src/overlay.jsx Outdated
Copy link
Member

Choose a reason for hiding this comment

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

You removed the function call to this method.
So I think that we can remove preventScrolling and allowScrolling method.

This component is also used by LeftNav, I have just checked, it should be fine.
I think that it will also be good to move _allowScrollingand _preventScrolling inside _applyAutoLockScrolling since their are only called by this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

componentWillUnmount calls _allowScrolling, had a think about combining, but think its best to keep this separate. Does that sound ok?

@oliviertassinari
Copy link
Member

@chrismcv I think that I'm done with my comments and that we can soon merge 😄.

@chrismcv chrismcv force-pushed the dialog-r2l branch 2 times, most recently from 365e81b to c01336d Compare November 21, 2015 18:29
@chrismcv
Copy link
Contributor Author

@oliviertassinari - defaultOpen added back in, and squashed.

@oliviertassinari
Copy link
Member

I have noticed one issue. When we open the dialog for a second time, the height is not correct.
I think that it's linked to the render-to-layout component since this bug only happen if you reopen the dialog at max one second after.

@chrismcv
Copy link
Contributor Author

@oliviertassinari apologies - rebase caused some confusion there, hence open and close. I had another rebase issue so manually merged it (see b4b7bb9).

Does this fix your height issue?

@oliviertassinari
Copy link
Member

@chrismcv The height issue is gone, great work 👍!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: dialog Changes related to the dialog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants