fix(Input): replace disabled class with disabled attribute #1591
Conversation
src/lib/htmlInputPropsUtils.js
Outdated
| 'multiple', 'name', 'pattern', 'placeholder', 'readOnly', 'required', 'step', 'type', 'value', | ||
|
|
||
| // Heads Up! |
There was a problem hiding this comment.
Handled this by not applying the disabled class. Potentially breaking change for users who relied on that as a CSS selector.
dvdzkwsk
force-pushed
the
fix/disabled-inputs
branch
from
57266d5
to
6373b23
Compare
Codecov Report
@@ Coverage Diff @@
## master #1591 +/- ##
==========================================
+ Coverage 99.74% 99.75% +<.01%
==========================================
Files 141 141
Lines 2398 2406 +8
==========================================
+ Hits 2392 2400 +8
Misses 6 6
Continue to review full report at Codecov.
|
|
Per offline chats, let's keep both the top level className so child elements are disabled but also apply the attribute. I'll PR the CSS to handle the double opacity issue. |
levithomason
force-pushed
the
fix/disabled-inputs
branch
from
6373b23
to
6c02a12
Compare
|
Tracking Semantic-Org/Semantic-UI#5284 |
|
This seems to have broken tests for |
|
SUI issue has no traction :( |
…React into fix/disabled-inputs # Conflicts: # src/lib/htmlInputPropsUtils.js
|
|
||
| if (disabled) htmlInputProps.disabled = disabled | ||
| if (onChange) htmlInputProps.onChange = this.handleChange | ||
| if (tabIndex) htmlInputProps.tabIndex = tabIndex |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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?
|
Thanks for the updates @layershifter ! |
|
Released in |
Resolves #1565.
@levithomason This, ironically enough, is a matter of semantics. Your call here, but in my opinion this improves the functionality of inputs as they now receive an actual
disabledattribute instead of relying on custom styling with adisabledclass above the input element itself. This allows disabled inputs to inherit standard browser behavior and conform to general user expectations.I checked it out in the docs and by removing the
disabledclass, the styles still look correct.