Skip to content

[Form components] Set cursor:not-allowed style when disabled #4170

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

Merged
merged 1 commit into from
Jun 19, 2016
Merged

[Form components] Set cursor:not-allowed style when disabled #4170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions src/Checkbox/Checkbox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,11 @@ function getStyles(props, context) {
},
checkWhenDisabled: {
fill: checkbox.disabledColor,
cursor: 'not-allowed',
},
boxWhenDisabled: {
fill: props.checked ? 'transparent' : checkbox.disabledColor,
cursor: 'not-allowed',
},
label: {
color: props.disabled ? checkbox.labelDisabledColor : checkbox.labelColor,
Expand Down
2 changes: 1 addition & 1 deletion src/DatePicker/DateDisplay.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ function getStyles(props, context, state) {
marginBottom: 10,
},
yearTitle: {
cursor: (!selectedYear && !props.disableYearSelection) ? 'pointer' : 'default',
cursor: props.disableYearSelection ? 'not-allowed' : (!selectedYear ? 'pointer' : 'default'),
Copy link
Member

@oliviertassinari oliviertassinari Jun 4, 2016

Choose a reason for hiding this comment

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

Do we really need a not-allowed cursor here?
What would be the use case?

Copy link
Author

@mykyta-shulipa mykyta-shulipa Jun 5, 2016

Choose a reason for hiding this comment

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

From docs:
disableYearSelection - Disables the year selection in the date picker.

I thinks, this is the use-case.
Could you please explain, why this is not the use-case?

Copy link
Member

@oliviertassinari oliviertassinari Jun 5, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Changing the disableYearSelection property doesn't change the styling of the element.
Hence, I don't think that we should change the cursor.

Why should we use the not-allowed? I think that we should us it to emphasize the disabled state.
So when should we use not-allowed? I would say as soon as we change the default style.

Copy link
Author

@mykyta-shulipa mykyta-shulipa Jun 5, 2016

Choose a reason for hiding this comment

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

Changing the disableYearSelection property doesn't change the styling of the element.

good point, agree. But I think, in this particular case we should emphasize disabled case.
Property disableYearSelection can be dependent on some custom application component property.
And this is case for the emphasizing - in come cases this field will be true, in some false. And year field in UI will be interactive or not.

Copy link
Member

@oliviertassinari oliviertassinari Jun 5, 2016

Choose a reason for hiding this comment

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

Good point. I agree as the default value of disableYearSelection is false 👍 .

},
};

Expand Down
1 change: 1 addition & 0 deletions src/IconButton/IconButton.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ function getStyles(props, context) {
disabled: {
color: baseTheme.palette.disabledColor,
fill: baseTheme.palette.disabledColor,
cursor: 'not-allowed',
},
};
}
Expand Down
1 change: 1 addition & 0 deletions src/MenuItem/MenuItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ function getStyles(props, context) {
const styles = {
root: {
color: props.disabled ? disabledColor : textColor,
cursor: props.disabled ? 'not-allowed' : 'inherit',
lineHeight: props.desktop ? '32px' : '48px',
fontSize: props.desktop ? 15 : 16,
whiteSpace: 'nowrap',
Expand Down
2 changes: 2 additions & 0 deletions src/RadioButton/RadioButton.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,11 @@ function getStyles(props, context) {
},
targetWhenDisabled: {
fill: radioButton.disabledColor,
cursor: 'not-allowed',
},
fillWhenDisabled: {
fill: radioButton.disabledColor,
cursor: 'not-allowed',
},
label: {
color: props.disabled ? radioButton.labelDisabledColor : radioButton.labelColor,
Expand Down
1 change: 1 addition & 0 deletions src/SelectField/SelectField.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ class SelectField extends Component {
<TextField
{...other}
style={style}
disabled={disabled}
floatingLabelFixed={floatingLabelFixed}
floatingLabelText={floatingLabelText}
floatingLabelStyle={floatingLabelStyle}
Expand Down
1 change: 1 addition & 0 deletions src/Stepper/StepLabel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ const getStyles = ({active, completed, disabled}, {muiTheme, stepper}) => {
if (disabled) {
styles.icon.color = inactiveIconColor;
styles.root.color = disabledTextColor;
styles.root.cursor = 'not-allowed';
}

return styles;
Expand Down
8 changes: 6 additions & 2 deletions src/Table/TableBody.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,12 +190,13 @@ class TableBody extends Component {
if (!this.props.displayRowCheckbox) return null;

const key = `${rowProps.rowNumber}-cb`;
const disabled = !this.props.selectable;
const checkbox = (
<Checkbox
ref="rowSelectCB"
name={key}
value="selected"
disabled={!this.props.selectable}
disabled={disabled}
checked={rowProps.selected}
/>
);
Expand All @@ -204,7 +205,10 @@ class TableBody extends Component {
<TableRowColumn
key={key}
columnNumber={0}
style={{width: 24}}
style={{
width: 24,
cursor: disabled ? 'not-allowed' : 'inherit',
}}
>
{checkbox}
</TableRowColumn>
Expand Down
22 changes: 19 additions & 3 deletions src/Table/TableHeader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,27 +126,43 @@ class TableHeader extends Component {
getCheckboxPlaceholder(props) {
if (!this.props.adjustForCheckbox) return null;

const disabled = !this.props.enableSelectAll;
const key = `hpcb${props.rowNumber}`;
return <TableHeaderColumn key={key} style={{width: 24}} />;
return (
<TableHeaderColumn
key={key}
style={{
width: 24,
cursor: disabled ? 'not-allowed' : 'inherit',
}}
/>
);
}

getSelectAllCheckboxColumn(props) {
if (!this.props.displaySelectAll) return this.getCheckboxPlaceholder(props);

const disabled = !this.props.enableSelectAll;
const checkbox = (
<Checkbox
key="selectallcb"
name="selectallcb"
value="selected"
disabled={!this.props.enableSelectAll}
disabled={disabled}
checked={this.props.selectAllSelected}
onCheck={this.handleCheckAll}
/>
);

const key = `hpcb${props.rowNumber}`;
return (
<TableHeaderColumn key={key} style={{width: 24}}>
<TableHeaderColumn
key={key}
style={{
width: 24,
cursor: disabled ? 'not-allowed' : 'inherit',
}}
>
{checkbox}
</TableHeaderColumn>
);
Expand Down
2 changes: 1 addition & 1 deletion src/TextField/EnhancedTextarea.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ function getStyles(props, context, state) {
resize: 'none',
font: 'inherit',
padding: 0,
cursor: props.disabled ? 'default' : 'initial',
cursor: props.disabled ? 'not-allowed' : 'initial',
},
shadow: {
resize: 'none',
Expand Down
1 change: 1 addition & 0 deletions src/TextField/TextField.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ const getStyles = (props, context, state) => {
outline: 'none',
backgroundColor: 'rgba(0,0,0,0)',
color: props.disabled ? disabledTextColor : textColor,
cursor: props.disabled ? 'not-allowed' : 'initial',
Copy link

@TildeWill TildeWill Nov 3, 2016

Choose a reason for hiding this comment

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

'initial' is not supported by IE. This means that if I start with a disabled TextField, and later enable it, the cursor is still not-allowed. auto is also pretty aggressive for a library. Can the cursor style be removed entirely alternatively? http://www.w3schools.com/cssref/css_initial.asp

Copy link
Author

@mykyta-shulipa mykyta-shulipa Nov 4, 2016

Choose a reason for hiding this comment

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

@TildeWill
maybe, it will be good to create new issue (and pull request) for this since this pull-request was already merged few versions ago?

Copy link
Member

@oliviertassinari oliviertassinari Nov 5, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

auto is also pretty aggressive for a library.

@TildeWill Could you develop on this point? auto is the initial value for the cursor property.
However, the cursor inherit. So using auto is going to break the inheritance chain.
Which sounds like a good thing for isolation purposes of our components.

We would definitely accept a PR for this.

font: 'inherit',
},
textarea: {},
Expand Down
2 changes: 1 addition & 1 deletion src/TextField/TextFieldLabel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ function getStyles(props) {
top: 38,
transition: transitions.easeOut(),
zIndex: 1, // Needed to display label above Chrome's autocomplete field background
cursor: props.disabled ? 'default' : 'text',
cursor: props.disabled ? 'not-allowed' : 'text',
transform: 'scale(1) translate3d(0, 0, 0)',
transformOrigin: 'left top',
pointerEvents: 'auto',
Expand Down
1 change: 1 addition & 0 deletions src/TextField/TextFieldUnderline.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ const TextFieldUnderline = (props) => {
disabled: {
borderBottom: 'dotted 2px',
borderColor: disabledTextColor,
cursor: 'not-allowed',
},
focus: {
borderBottom: 'solid 2px',
Expand Down
3 changes: 3 additions & 0 deletions src/Toggle/Toggle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,16 @@ function getStyles(props, context, state) {
},
trackWhenDisabled: {
backgroundColor: toggle.trackDisabledColor,
cursor: 'not-allowed',
},
thumbWhenDisabled: {
backgroundColor: toggle.thumbDisabledColor,
cursor: 'not-allowed',
},
label: {
color: disabled ? toggle.labelDisabledColor : toggle.labelColor,
width: `calc(100% - ${(toggleTrackWidth + 10)}px)`,
cursor: disabled ? 'not-allowed' : 'initial',
},
};

Expand Down