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

Jump to bottom

fix(Input): replace disabled class with disabled attribute #1591

Merged
merged 5 commits into from
May 6, 2017
Merged

fix(Input): replace disabled class with disabled attribute #1591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
57266d5
fix(Input): replace disabled class with disabled attribute
Apr 17, 2017
6c02a12
fix(Input): restore disabled className
levithomason Apr 17, 2017
f2756ce
refactor(Input): cleanup handling of `disabled`
levithomason Apr 17, 2017
0706983
fix(Input): small refactoring to make tests running
Apr 25, 2017
0cdeee1
Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…
Apr 25, 2017
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
55 changes: 34 additions & 21 deletions src/elements/Input/Input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,13 @@ class Input extends Component {
type: META.TYPES.ELEMENT,
}

computeTabIndex = () => {
const { disabled, tabIndex } = this.props

if (!_.isNil(tabIndex)) return tabIndex
if (disabled) return -1
}

focus = () => (this.inputRef.focus())

handleChange = (e) => {
Expand All @@ -135,6 +142,23 @@ class Input extends Component {

handleInputRef = c => (this.inputRef = c)

partitionProps = () => {
const { disabled, onChange, type } = this.props

const tabIndex = this.computeTabIndex()
const unhandled = getUnhandledProps(Input, this.props)
const [htmlInputProps, rest] = partitionHTMLInputProps(unhandled)

htmlInputProps.ref = this.handleInputRef
htmlInputProps.type = type

if (disabled) htmlInputProps.disabled = disabled
if (onChange) htmlInputProps.onChange = this.handleChange
if (tabIndex) htmlInputProps.tabIndex = tabIndex
Copy link
Member

@layershifter layershifter Apr 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The problem is that the component receives disabled as undefined, same is for tabIndex

I made tests working, however, it's not the best idea 💩 May be better solution possible there?

Copy link
Member

Choose a reason for hiding this comment

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

I believe if the disabled prop is undefined, then the htmlInputProps.disabled should also be undefined. Perhaps I'm overlooking the issue or use case here.

Copy link
Member

Choose a reason for hiding this comment

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

@levithomason You're right, but if we replace current code with:

if (onChange) htmlInputProps.onChange = this.handleChange

return [{...htmlInputProps, disabled, tabIndex}, rest]

But, we will have failing tests of shorhand.

Copy link
Member

Choose a reason for hiding this comment

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

However, we can fix this in test:

  common.implementsHTMLInputProp(Input, {
    alwaysPresent: true,
-    shorthandDefaultProps: { type: 'text' },
+    shorthandDefaultProps: { disabled: undefined, tabIndex: undefined, type: 'text' },
  })

May be I overlooked something?


return [htmlInputProps, rest]
}

render() {
const {
action,
Expand All @@ -152,9 +176,7 @@ class Input extends Component {
label,
labelPosition,
loading,
onChange,
size,
tabIndex,
transparent,
type,
} = this.props
Expand All @@ -175,26 +197,15 @@ class Input extends Component {
'input',
className,
)
const unhandled = getUnhandledProps(Input, this.props)
const ElementType = getElementType(Input, this.props)

// Heads up! We should pass `type` prop manually because `Input` component handles it
const [htmlInputProps, rest] = partitionHTMLInputProps({ ...unhandled, type })

if (onChange) htmlInputProps.onChange = this.handleChange
htmlInputProps.ref = this.handleInputRef

// tabIndex
if (!_.isNil(tabIndex)) htmlInputProps.tabIndex = tabIndex
else if (disabled) htmlInputProps.tabIndex = -1
const [htmlInputProps, rest] = this.partitionProps()

// Render with children
// ----------------------------------------
if (!_.isNil(children)) {
// add htmlInputProps to the `<input />` child
const childElements = _.map(Children.toArray(children), (child) => {
if (child.type !== 'input') return child

return cloneElement(child, this.handleChildOverrides(child, htmlInputProps))
})

Expand All @@ -205,13 +216,15 @@ class Input extends Component {
// ----------------------------------------
const actionElement = Button.create(action, { defaultProps: { className: 'button' } })
const iconElement = Icon.create(icon)
const labelElement = Label.create(label, { defaultProps: {
className: cx(
'label',
// add 'left|right corner'
_.includes(labelPosition, 'corner') && labelPosition,
),
} })
const labelElement = Label.create(label, {
defaultProps: {
className: cx(
'label',
// add 'left|right corner'
_.includes(labelPosition, 'corner') && labelPosition,
),
},
})

return (
<ElementType {...rest} className={classes}>
Expand Down
8 changes: 2 additions & 6 deletions src/lib/htmlInputPropsUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,8 @@ export const htmlInputAttrs = [
'selected', 'defaultValue', 'defaultChecked',

// LIMITED HTML PROPS
'autoCapitalize', 'autoComplete', 'autoCorrect', 'autoFocus', 'checked', 'form', 'id', 'max', 'maxLength', 'min',
'multiple', 'name', 'pattern', 'placeholder', 'readOnly', 'required', 'step', 'type', 'value',

// Heads Up!
// Do not pass disabled, it duplicates the SUI CSS opacity rule.
// 'disabled',
'autoCapitalize', 'autoComplete', 'autoCorrect', 'autoFocus', 'checked', 'disabled', 'form', 'id', 'max', 'maxLength',
'min', 'multiple', 'name', 'pattern', 'placeholder', 'readOnly', 'required', 'step', 'type', 'value',
]

export const htmlInputEvents = [
Expand Down
12 changes: 12 additions & 0 deletions test/specs/elements/Input/Input-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,18 @@ describe('Input', () => {
})
})

describe('disabled', () => {
it('is applied to the underlying html input element', () => {
shallow(<Input disabled />)
.find('input')
.should.have.prop('disabled', true)

shallow(<Input disabled={false} />)
.find('input')
.should.have.not.prop('disabled')
})
})

describe('tabIndex', () => {
it('is not set by default', () => {
shallow(<Input />)
Expand Down