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.

Sign up for GitHub

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

Fixed Checkbox and password cursor state while hiding/showing #13317

Closed
wants to merge 4 commits into from
Closed

Fixed Checkbox and password cursor state while hiding/showing #13317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e9dca7a
Fixed regression
esh-g Dec 4, 2022
f1974f2
Fixed regression
esh-g Dec 4, 2022
9d66500
Refactored checkbox
esh-g Dec 7, 2022
8db9a5a
Merge branch 'main' into someone-here-password-cursor
esh-g Dec 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 31 additions & 17 deletions src/components/Checkbox.js → src/components/Checkbox/BaseCheckbox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,23 @@
import React from 'react';
import {View, Pressable} from 'react-native';
import PropTypes from 'prop-types';
import styles from '../styles/styles';
import themeColors from '../styles/themes/default';
import stylePropTypes from '../styles/stylePropTypes';
import Icon from './Icon';
import * as Expensicons from './Icon/Expensicons';
import styles from '../../styles/styles';
import themeColors from '../../styles/themes/default';
import stylePropTypes from '../../styles/stylePropTypes';
import Icon from '../Icon';
import * as Expensicons from '../Icon/Expensicons';

const requiredPropsCheck = (props, componentName) => {
if (props.onMouseDown || props.onPress) {
return;
}
return new Error(`One of "onMouseDown" or "onPress" must be provided in ${componentName}`);
};

const propTypes = {
/** Whether checkbox is checked */
isChecked: PropTypes.bool,

/** A function that is called when the box/label is pressed */
onPress: PropTypes.func.isRequired,

/** Should the input be styled for errors */
hasError: PropTypes.bool,

Expand All @@ -26,14 +30,17 @@ const propTypes = {
/** Additional styles to add to checkbox button */
style: stylePropTypes,

/** Callback that is called when mousedown is triggered. */
onMouseDown: PropTypes.func,

/** A ref to forward to the Pressable */
forwardedRef: PropTypes.oneOfType([
PropTypes.func,
PropTypes.shape({current: PropTypes.instanceOf(React.Component)}),
]),

/** A function that is called when the box/label is pressed */
onPress: requiredPropsCheck,

/** Callback that is called when mousedown is triggered. */
onMouseDown: requiredPropsCheck,
};

const defaultProps = {
Expand All @@ -44,9 +51,10 @@ const defaultProps = {
forwardedRef: undefined,
children: null,
onMouseDown: undefined,
onPress: undefined,
};

class Checkbox extends React.Component {
class BaseCheckbox extends React.Component {
constructor(props) {
super(props);
this.state = {
Expand All @@ -72,7 +80,11 @@ class Checkbox extends React.Component {
return;
}

this.props.onPress();
if (this.props.onPress) {
this.props.onPress(event);
} else {
this.props.onMouseDown(event);
}
}

firePressHandlerOnClick(event) {
Expand All @@ -89,7 +101,9 @@ class Checkbox extends React.Component {
this.onBlur();
}

this.props.onPress();
if (this.props.onPress) {
this.props.onPress(event);
}
}

render() {
Expand Down Expand Up @@ -129,7 +143,7 @@ class Checkbox extends React.Component {
}
}

Checkbox.propTypes = propTypes;
Checkbox.defaultProps = defaultProps;
BaseCheckbox.propTypes = propTypes;
BaseCheckbox.defaultProps = defaultProps;

export default Checkbox;
export default BaseCheckbox;
17 changes: 17 additions & 0 deletions src/components/Checkbox/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import _ from 'lodash';
import React from 'react';
import BaseCheckbox from './BaseCheckbox';

const Checkbox = props => (
<BaseCheckbox
// eslint-disable-next-line react/jsx-props-no-spreading
{..._.omit(props, 'onPress')}
onMouseDown={props.onMouseDown || props.onPress}
/>
);

Checkbox.propTypes = BaseCheckbox.propTypes;
Checkbox.defaultProps = BaseCheckbox.defaultProps;
Checkbox.displayName = 'Checkbox';

export default Checkbox;
17 changes: 17 additions & 0 deletions src/components/Checkbox/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import _ from 'lodash';
import React from 'react';
import BaseCheckbox from './BaseCheckbox';

const Checkbox = props => (
<BaseCheckbox
// eslint-disable-next-line react/jsx-props-no-spreading
{..._.omit(props, 'onMouseDown')}
onPress={props.onPress || props.onMouseDown}
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to not mention this before, but I wonder why onMouseDown is necessary in the native Component? Perhaps a comment would help clear up why we need both to anyone looking at this in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// If no onPress is passed, then execute the onMouseDown function
Is this comment ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I add a commit with this comment?

Copy link
Contributor

@Julesssss Julesssss Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It just doesn't make much sense to me why native components would ever need onMouseDown. Couldn't it be removed entirely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is actually essential. You see what I'm doing here is not executing any code on the onMouseDown event, but if the onMouseDown prop is passed, then I am executing the prop passed on the onPress event because the onMouseDown doesn't fire in native.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense to me for the general component, but there will never be an onMouseDown event for the native component, right? Or am I missunderstanding

Copy link
Contributor Author

@esh-g esh-g Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes there will never be an onMouseDown According to my knowledge. You are you proposing to remove the omit from the props spread? I had just added it to eliminate any chance of double event firing in any edge case or such... At first I thought you were referring to the onPress={props.onPress || props.onMouseDown}, that line is essential but the omit is not essential, it just to make sure no edge cases or double event firing happens...

Copy link
Contributor

Choose a reason for hiding this comment

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

At first I thought you were referring to the onPress={props.onPress || props.onMouseDown}, that line is essential

Oh yeah, sorry that is what I meant. It would be great to add a comment that explains why this is necessary --- to help people like me who are a bit confused by seeing it there.

Copy link
Contributor Author

@esh-g esh-g Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Every checkbox can have two click handlers onPress and onMouseDown. As there is no onMouseDown event on native, any prop that is passed to be executed on the onMouseDown event should be executed on the onPress event instead.
Should I add this as a multiline comment above that line?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @someone-here. This has made me realise that the solution is just a bit too complex for solving a small issue. Let's drop this one and move onto other bugs 👍

/>
);

Checkbox.propTypes = BaseCheckbox.propTypes;
Checkbox.defaultProps = BaseCheckbox.defaultProps;
Checkbox.displayName = 'Checkbox';

export default Checkbox;
9 changes: 6 additions & 3 deletions src/components/TextInput/BaseTextInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,11 @@ class BaseTextInput extends Component {
]).start();
}

togglePasswordVisibility() {
togglePasswordVisibility(event) {
if (!event) {
return;
}
event.preventDefault();
this.setState(prevState => ({passwordHidden: !prevState.passwordHidden}));
}

Expand Down Expand Up @@ -291,8 +295,7 @@ class BaseTextInput extends Component {
{this.props.secureTextEntry && (
<Checkbox
style={styles.secureInputShowPasswordButton}
onPress={this.togglePasswordVisibility}
onMouseDown={e => e.preventDefault()}
onMouseDown={this.togglePasswordVisibility}
>
<Icon
src={this.state.passwordHidden ? Expensicons.Eye : Expensicons.EyeDisabled}
Expand Down