-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Fix for: mWeb/Chrome - Payment - The keyboard overlaps the "Make default payment method" button #14725
Fix for: mWeb/Chrome - Payment - The keyboard overlaps the "Make default payment method" button #14725
Changes from 13 commits
2fc343c
f4a1d49
a5cd3b9
8c260b5
a3cc328
663c71b
076c4dc
d011e99
99bd6a7
b09f1eb
bd422a2
1359e05
acca535
483bf82
34401d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ import TextInput from '../TextInput'; | |
import KeyboardSpacer from '../KeyboardSpacer'; | ||
import {propTypes as passwordPopoverPropTypes, defaultProps as passwordPopoverDefaultProps} from './passwordPopoverPropTypes'; | ||
import Button from '../Button'; | ||
import withViewportOffsetTop from '../withViewportOffsetTop'; | ||
|
||
const propTypes = { | ||
/** Whether we should wait before focusing the TextInput, useful when using transitions on Android */ | ||
|
@@ -55,6 +56,7 @@ class BasePasswordPopover extends Component { | |
onClose={this.props.onClose} | ||
anchorPosition={this.props.anchorPosition} | ||
onModalShow={this.focusInput} | ||
extraModalStyles={{maxHeight: this.props.windowHeight, marginTop: this.props.viewportOffsetTop}} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be careful to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aimane-chnaif Agreed, but this component is only used used in |
||
> | ||
<View | ||
style={[ | ||
|
@@ -98,6 +100,7 @@ class BasePasswordPopover extends Component { | |
BasePasswordPopover.propTypes = propTypes; | ||
BasePasswordPopover.defaultProps = defaultProps; | ||
export default compose( | ||
withViewportOffsetTop, | ||
withWindowDimensions, | ||
withLocalize, | ||
)(BasePasswordPopover); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
import React, {Component} from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import lodashGet from 'lodash/get'; | ||
import getComponentDisplayName from '../libs/getComponentDisplayName'; | ||
import addViewportResizeListener from '../libs/VisualViewport'; | ||
|
||
const viewportOffsetTopPropTypes = { | ||
// viewportOffsetTop returns the offset of the top edge of the visual viewport from the | ||
// top edge of the layout viewport in CSS pixels, when the visual viewport is resized. | ||
|
||
viewportOffsetTop: PropTypes.number.isRequired, | ||
jasperhuangg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
|
||
export default function (WrappedComponent) { | ||
class WithViewportOffsetTop extends Component { | ||
constructor(props) { | ||
super(props); | ||
|
||
this.updateDimensions = this.updateDimensions.bind(this); | ||
|
||
this.state = { | ||
viewportOffsetTop: 0, | ||
}; | ||
} | ||
|
||
componentDidMount() { | ||
this.removeViewportResizeListener = addViewportResizeListener(this.updateDimensions); | ||
} | ||
|
||
componentWillUnmount() { | ||
this.removeViewportResizeListener(); | ||
} | ||
|
||
/** | ||
* @param {SyntheticEvent} e | ||
*/ | ||
updateDimensions(e) { | ||
const viewportOffsetTop = lodashGet(e, 'target.offsetTop', 0); | ||
this.setState({viewportOffsetTop}); | ||
} | ||
|
||
render() { | ||
return ( | ||
<WrappedComponent | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
{...this.props} | ||
ref={this.props.forwardedRef} | ||
viewportOffsetTop={this.state.viewportOffsetTop} | ||
/> | ||
); | ||
} | ||
} | ||
|
||
WithViewportOffsetTop.displayName = `WithViewportOffsetTop(${getComponentDisplayName(WrappedComponent)})`; | ||
WithViewportOffsetTop.propTypes = { | ||
forwardedRef: PropTypes.oneOfType([ | ||
PropTypes.func, | ||
PropTypes.shape({current: PropTypes.instanceOf(React.Component)}), | ||
]), | ||
}; | ||
WithViewportOffsetTop.defaultProps = { | ||
forwardedRef: undefined, | ||
}; | ||
return React.forwardRef((props, ref) => ( | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
<WithViewportOffsetTop {...props} forwardedRef={ref} /> | ||
)); | ||
} | ||
|
||
export { | ||
viewportOffsetTopPropTypes, | ||
}; |
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.
I don't really like the name
extraModalStyles
, can we come up with something a bit more descriptive?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.
Sure, any suggestions?
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.
@jasperhuangg customModalStyles?
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.
@aimane-chnaif Any ideas?
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.
Since this is already Modal, no need to label
Modal
I think.So maybe something like
customContainerStyle
?All yours @jasperhuangg
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.
Thanks for your patience guys have been out sick.
I like
outerStyle
? And I think we can renamecontainerStyle
toinnerContainerStyle
since the container those styles are being applied to lives inside the modal.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.
@jasperhuangg sorry, hope you'd feel better now.
Good suggestion. I agree with that approach. But how about
contentContainerStyle
since this name is more common and already used in other places?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.
@jasperhuangg No worries, hope you're feeling better.
outerStyle
andinnerContainerStyle
make sense to me, just give me the word and I'll implement these changes.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.
@jasperhuangg sorry bump, can you please confirm the preferred name soon? Hope delay for waiting this won't affect timeline
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.
@Ollyws yeah go ahead! Sorry for the delay