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

Fix using 2 portals #18

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Fix using 2 portals #18

wants to merge 7 commits into from

Conversation

jguffey
Copy link

@jguffey jguffey commented Mar 28, 2017

Fixes #8,

ToDo:
[ ] Clicking outside of the Menu should close the Dropdown Dialog.
[ ] Test with a dialog Polyfill

I had to nest the open menu within another <dialog> element, as the browser will not position anything over an open <dialog>, except for another open <dialog>.

Feedback is appreciated.

jguffey and others added 7 commits March 17, 2017 14:18
Create a more designed Menu Dropdown Button
8e1e5bc 
I had a similar use case on a project in the past, so I think it would be useful to have a dropdown that acts as a button.

Mostly it's just an example of what one can do with this library, but it might not be immediately apparent how to create one of these.

I also added `yarn.lock` to the .gitignore for those of us that want to use yarn.
Basically, within a dialog, we can't use this plugin, instead use the…
4319241 
… default MDL menu components
Finally got the dialog to render the dropdown.
55523a9 
Needs to be cleaned up before PR.

1. Because only <dialog> can be displayed over a <dialog>, we must render the menu list within a <dialog>
2. Various compolications with Portal, because of 1. we must also preserve Poral. This also perserves all other benefits of using this over mdl-react
3. Portal passes props.closePortal implicitly, thus we need DialogWrapper component to accept and pass this prop to children.

This seems to be working, I need to do more tests and complete the following items:

To do:

1. Split DialogWrapper into it's own file
2. Test cross browser
@jguffey
wip
9e8f238
removed unnessessary unmount handler
66850fd
@marcofalcioni
Merge pull request #2 from jguffey/add-dropdown-button
a8e83ab 
Create a more designed Menu Dropdown Button
@marcofalcioni
Merge branch 'master' into fix-using-2-portals
8a9a903
@zhenyanghua
Copy link

zhenyanghua commented Apr 27, 2017 •
edited
Loading

@jguffey, Here are the bugs I found so far:

  • The dropdown doesn't collapse when it is onBlur
  • Doesn't work in any other browsers other than Chrome.

It is prompted:

  • TypeError: this.dialogDom.showModal is not a function[Learn More] bundle.js:49302:8
  • TypeError: root is null[Learn More] bundle.js:47327:1
  • TypeError: this.dialogDom.showModal is not a function[Learn More]

@jguffey
Copy link
Author

jguffey commented Apr 27, 2017

@zhenyanghua true. @HriBB this project hasn't been updated in some time. If I can get this PR up to spec (passing use case tests that zhenyanghua pointed out, would you be interested in merging this PR?

@HriBB
Copy link
Owner

HriBB commented May 10, 2017

I am on a long vacation. Will merge when I get back in a week or so 😉 sorry for the delay.

@jguffey
Copy link
Author

jguffey commented May 11, 2017

@HriBB Oh! Thanks for checking in, in that case I'll try to address some of the issues @zhenyanghua brought up. I've been using this lib in production, but there are a few issues still holding it back.

@jguffey
Copy link
Author

jguffey commented May 11, 2017

The bugs with this PR seem to mostly be related to the use of the <dialog> component, which is not well supported outside of Chrome at this time. Using a polyfill should fix most of the issues @zhenyanghua pointed out. Personally I've had success with dialog-polyfill. One of my to-do items for this PR is to conduct more testing with this polyfill.

The other issue: "The dropdown doesn't collapse when it is onBlur", is also related to <dialog>. We want to detect a click on the pseudoelement ::backdrop, but according to this thread that approach may be troublesome in some browsers.

HriBB
HriBB requested changes Jun 6, 2017
View reviewed changes
Copy link
Owner

@HriBB HriBB left a comment

Choose a reason for hiding this comment

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

I tried your PR locally and it breaks all my menu demos in storybook. Positioning is broken and I cannot close menu by clicking outside. But menu opens correctly in dialog. If you can fix the code so that positioning and closing works.

@jguffey
Copy link
Author

jguffey commented Jun 7, 2017

It even breaks other demos? Thanks for checking, I'll look into that.

Clicking outside to close a Portal is a known issue, one I'm not entirely sure how to solve and may not be solvable given the ::backdrop problem explained in that thread I mentioned.

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

@HriBB HriBB HriBB requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SelectField doesn't work inside Dialog
4 participants
@jguffey @zhenyanghua @HriBB @marcofalcioni