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

17051 - migrated Checkbox to PressableWithFeedback #20257

Merged
merged 11 commits into from
Jun 29, 2023
Merged

17051 - migrated Checkbox to PressableWithFeedback #20257

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ac6b7d7
migrated Checkbox to PressableWithFeedback
Skalakid Jun 6, 2023
73859b7
Merge branch 'main' into 17051-migrate-Checkbox
Skalakid Jun 12, 2023
0690682
Review changes
Skalakid Jun 12, 2023
d16f038
Merge branch 'main' into 17051-migrate-Checkbox
Skalakid Jun 15, 2023
46c4130
removed hoverDimmingValue as it is set to 1 by default
Skalakid Jun 15, 2023
d25865e
Merge branch 'main' into 17051-migrate-Checkbox
Skalakid Jun 19, 2023
5d1e5ed
Merge branch 'main' into 17051-migrate-Checkbox
Skalakid Jun 21, 2023
4917bf7
fixed bug with password text input eye icon
Skalakid Jun 21, 2023
56b01f4
updated comment
Skalakid Jun 23, 2023
a63eee4
review changes + added accessibility labels
Skalakid Jun 27, 2023
0426c25
Merge branch 'main' into 17051-migrate-Checkbox
Skalakid Jun 29, 2023
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
16 changes: 12 additions & 4 deletions src/components/Checkbox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import React from 'react';
import {View, Pressable} from 'react-native';
import {View} 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 PressableWithFeedback from './Pressable/PressableWithFeedback';

const propTypes = {
/** Whether checkbox is checked */
Expand Down Expand Up @@ -34,6 +35,9 @@ const propTypes = {

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

/** A ref to forward to the Pressable */
Copy link
Contributor

Choose a reason for hiding this comment

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

@Skalakid Wrong type comment here. Could you update this?

accessibilityLabel: PropTypes.string,
};

const defaultProps = {
Expand All @@ -45,6 +49,7 @@ const defaultProps = {
forwardedRef: undefined,
children: null,
onMouseDown: undefined,
accessibilityLabel: 'checkbox',
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the default accessibilityLabel be empty string? Since it will not be useful to read out checkbox without context in case of default is not useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I will change that

};

class Checkbox extends React.Component {
Expand Down Expand Up @@ -95,20 +100,23 @@ class Checkbox extends React.Component {

render() {
return (
<Pressable
<PressableWithFeedback
disabled={this.props.disabled}
onPress={this.firePressHandlerOnClick}
onMouseDown={this.props.onMouseDown}
onFocus={this.onFocus}
onBlur={this.onBlur}
ref={this.props.forwardedRef}
onPressOut={this.onBlur}
Copy link
Contributor

Choose a reason for hiding this comment

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

@Skalakid What is this this.onBlur? I always get undefined value for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Skalakid any update on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abdulrahuman5196 Hey, sorry for the late response, I just fixed this by merging with the main ;) I think it was added after merging with main, now it's gone

style={this.props.style}
style={[this.props.style, styles.userSelectNone]}
Copy link
Contributor

Choose a reason for hiding this comment

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

And any reason to not allow user to select the checkbox text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously after double-clicking the checkbox icon was somehow selectable, now after I merged the latest main I can't reproduce it so I can remove it

onKeyDown={this.handleSpaceKey}
accessibilityRole="checkbox"
accessibilityState={{
checked: this.props.isChecked,
}}
accessibilityLabel={this.props.accessibilityLabel}
hoverDimmingValue={1}
Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove this? Since currently the default for hoverDimmingValue is 1?

pressDimmingValue={1}
>
{this.props.children ? (
this.props.children
Expand All @@ -133,7 +141,7 @@ class Checkbox extends React.Component {
)}
</View>
)}
</Pressable>
</PressableWithFeedback>
);
}
}
Expand Down