-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Removes the short-lived tether dependency in react-dates #163
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - couple linter comments. also it'd be good to at least update the PR description to explain in detail why tether didn't work for us.
@@ -378,7 +424,11 @@ export default class DateRangePicker extends React.Component { | |||
const onOutsideClick = !withFullScreenPortal ? this.onOutsideClick : undefined; | |||
|
|||
return ( | |||
<div className={this.getDayPickerContainerClasses()}> | |||
<div | |||
ref={ref => { this.dayPickerContainer = ref; }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the linter should be requiring arg parens here
@@ -224,7 +268,11 @@ export default class SingleDatePicker extends React.Component { | |||
const onOutsideClick = !withFullScreenPortal ? this.onClearFocus : undefined; | |||
|
|||
return ( | |||
<div className={this.getDayPickerContainerClasses()}> | |||
<div | |||
ref={ref => { this.dayPickerContainer = ref; }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also here
I think this is actually going to be a major bump because of the changes in DOM structure and classnames. |
const { dayPickerContainerStyles } = this.state; | ||
|
||
const containerClientRect = this.dayPickerContainer.getBoundingClientRect(); | ||
const documentBodyStyles = window.getComputedStyle(document.body); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call is fairly expensive because it traverses the entire cascade and builds CSSDeclaration. Maybe we can call responsivizePickerPosition
inside requestAnimationFrame
, what do you think?
b5c472b
to
30cb93a
Compare
…just doing the thing internally
30cb93a
to
5799674
Compare
Most of these additions cannot really be well tested without DOM manipulation so I'm going to merge this in as is. :/ |
Instead, we're just going to do the thing internally.
Fix for #139 as well.
to: @moonboots @airbnb/webinfra
Ugh