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

[DatePicker] data-picker-inline Popover instead of Paper #2557

Closed
wants to merge 1 commit into from

Conversation

tyfoo
Copy link
Contributor

@tyfoo tyfoo commented Dec 16, 2015

@oliviertassinari
Copy link
Member

Could you add onRequestClose as a required value in the propTypes definition?
This is what makes the clickaway feature work. I think that it's better to be explicit.

The Popover is not correctly positioned when opened.
It seems that we have an issue with the anchorEl property.
@chrismcv any tips?

<div style={styles.subContainer}>
<Paper {...other}>
<div style={styles.subContainer} ref="anchor">
<Popover open={open} anchorEl={this.refs.anchor} {...other}>
Copy link
Member

Choose a reason for hiding this comment

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

Can you put the {...other} at the beginning? I think that it's better in general, you don't want specific properties to be overridden.

@oliviertassinari oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Dec 16, 2015
@chrismcv
Copy link
Contributor

Only thing I can think of is that the ref won't exist on the first render, so if that happens to be the only render, then you're effectively passing null to the anchorEl prop.

@oliviertassinari
Copy link
Member

Only thing I can think of is that the ref won't exist on the first render, so if that happens to be the only render, then you're effectively passing null to the anchorEl prop.

That's true. However, when the popover is opened and rerendered, the ref is valid, still the position is wrong.
Could this is because of a wrong update of the ref position?

@tyfoo tyfoo force-pushed the feature/DatePicker-inline-popover branch from b6bc2b7 to 91cf8e1 Compare December 17, 2015 10:00
@tyfoo
Copy link
Contributor Author

tyfoo commented Dec 17, 2015

@oliviertassinari I've moved {...others} to the beginning.

In regards to onRequestClose, where would you like me to add it? the date-picker-inline shouldn't be used directly? I can add onRequestClose as required for DatePicker and then pass it down?

I've noticed that Popover already has a default onRequestClose and that ClickAway seems to work as is.

@chrismcv
Copy link
Contributor

you'll need it, as if the state is reset externally then open prop could be wrong. Specifically, the inline date picker should handle onRequestClose to manage this open state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants