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

Add support for acceptFirstMouse on window #4

Merged
merged 3 commits into from
Apr 20, 2017

Conversation

Vlemert
Copy link
Contributor

@Vlemert Vlemert commented Apr 15, 2017

Hey man!

Stumbled upon this project yesterday and I think this is a great idea! Figured I couldn't come empty handed, so here's my first attempt at a contribution. I immediately ran into some interesting stuff because acceptFirstMouse can only be passed as an option in the constructor of Window, which made me think that it only supports being 'uncontrolled'... I think...

Had some trouble testing this as well, so that's still missing. I didn't want to spend a lot of time on that before checking whether this prop is a good idea in the first place. Maybe you were planning on passing the initial options in a different way?

Anyway, love the idea, I'm definitely going to dive into how you got this to work.

@mhink
Copy link
Owner

mhink commented Apr 15, 2017

Thanks for the PR!

I didn't want to spend a lot of time on that before checking whether this prop is a good idea in the first place. Maybe you were planning on passing the initial options in a different way?

There's not really any other way to pass them in, truth be told, so I think this makes sense. Considering that there's no "controlled" analogue, I would just name the prop acceptFirstMouse and call it a day.

That said, could you add a case for this under WindowElement.commitUpdate which emits a warning if the user changed this prop? I'd like them to know that <window acceptFirstMouse /> is how it's meant to be used.

As far as testing: I totally feel you. I'll tell you what- if you can add a file under the examples/ directory, I'll figure out the "proper" testing (don't feel bad, I still need to add tests for position and size, haha)

@Vlemert
Copy link
Contributor Author

Vlemert commented Apr 16, 2017

No problem, happy to help!

So I added an example, removed 'default', and put in probably the lamest warning ever. Totally wasn't sure about how you were planning to do that stuff so I thought I'd just throw it in and we could discuss it. Maybe it's a good idea to put those checks behind a gate so it doesn't run in production, but it seems a mechanism like that is not in there yet.

About the testing, I think I have found out why I had trouble with that, and might have some ideas on how to make it easier. If you're interested I could make a separate issue to share my ideas.

- added stack
- check environment before logging
@Vlemert Vlemert mentioned this pull request Apr 19, 2017
@Vlemert
Copy link
Contributor Author

Vlemert commented Apr 19, 2017

Did some more digging in the react codebase and added a stacktrace to the warning message. Went for checking process.env.NODE_ENV before logging, just to be sure.

I created an issue for the stuff I ran into when trying to test this.

@mhink
Copy link
Owner

mhink commented Apr 20, 2017

Dude, you're a champion. Out of curiosity, what does getFiberStackAddendum do?

@mhink mhink merged commit ac9051c into mhink:master Apr 20, 2017
@Vlemert
Copy link
Contributor Author

Vlemert commented Apr 20, 2017

I found it being used here (and in the files next to that one).

It returns a sort of stack trace of where we are for the current render. If I trigger the warning in the ionize-example-app you'll get this:

image

I'm not really sure it's perfect like this, but I figured it's better than nothing. I'm still planning on debugging react to see how they use this and what it does over there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants