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

[Popper] Fix anchorEl prop types #16004

Merged
merged 5 commits into from
Jun 8, 2019
Merged

[Popper] Fix anchorEl prop types #16004

merged 5 commits into from
Jun 8, 2019

Conversation

dan8f
Copy link
Contributor

@dan8f dan8f commented Jun 2, 2019

@oliviertassinari, quick PR to discuss your proposal #15922

Closes #15922

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

I think we can remove any notion of Element now. Typing anchorEl structurally with the ReferenceObject seems sufficient. Element is just an example implementation of the ReferenceObject interface.

@eps1lon eps1lon added bug 🐛 Something doesn't work component: Popper The React component. See <Popup> for the latest version. labels Jun 3, 2019
@oliviertassinari
Copy link
Member

@eps1lon I haven't thought about this approach. Yeah, that sounds fair 👍.

@mui-pr-bot
Copy link

mui-pr-bot commented Jun 3, 2019

No bundle size changes comparing 64dab48...7c4f7ae

Generated by 🚫 dangerJS against 7c4f7ae

@oliviertassinari oliviertassinari requested a review from eps1lon June 4, 2019 17:39
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Would only mention AnchorElement BikeshedMyNamePlease1 and link to the API docs describing that interface.

1We can't use AnchorElement because that points to an instance of <a />. Maybe ReferenceObject is better after all.

@oliviertassinari
Copy link
Member

@eps1lon Maybe it's good enough as it is now?

@eps1lon
Copy link
Member

eps1lon commented Jun 5, 2019

@eps1lon Maybe it's good enough as it is now?

I mean you raised the concern so you should answer this. I was ok with ReferenceObject but offered another solution after concerns were raised.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 5, 2019

@eps1lon OK, after more digging, I'm fine with keeping ReferenceObject too :).

@oliviertassinari
Copy link
Member

I have updated the wording. Do you guys see something left to address?

@oliviertassinari
Copy link
Member

@dan8f It's a great first pull request on Material-UI 👌🏻. Thank you for working on it!

@eps1lon eps1lon mentioned this pull request Jun 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: Popper The React component. See <Popup> for the latest version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Popper faked reference object props validation fails
5 participants