fix(Dimmer): Fix "simple" usage, add disabled prop #795
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
14f588e to
673fd0d
Compare
levithomason
requested changes
Nov 5, 2016
| @@ -29,13 +29,13 @@ describe('Dimmer', () => { | |||
| }) | |||
|
|
|||
| describe('active', () => { | |||
| it('removes the `disabled` className when true', () => { | |||
| it('adds the `transition visible` className when true', () => { | |||
There was a problem hiding this comment.
The common test can be configured to assert a className that is different than the prop name. Should be able to replace this describe block with:
common.propKeyOnlyToClassName(Dimmer, 'active', {
className: 'active transition visible',
})| useKeyOnly(active, 'active'), | ||
| useKeyOnly(!active, 'disabled'), | ||
| useKeyOnly(active, 'active transition visible'), | ||
| useKeyOnly(disabled, 'disabled'), |
There was a problem hiding this comment.
How about adding a common test for this?
common.propKeyOnlyToClassName(Dimmer, 'disabled')673fd0d to
2ae5ce1
Compare
|
Added those tests and removed the ones for "active" that were testing duplicate functionality already covered by the common tests. Should be g2g 👍 |
Current coverage is 100% (diff: 100%)@@ master #795 diff @@
====================================
Files 136 136
Lines 2198 2198
Methods 0 0
Messages 0 0
Branches 0 0
====================================
Hits 2198 2198
Misses 0 0
Partials 0 0
|
levithomason
approved these changes
Nov 6, 2016
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I realized that #782 breaks the "simple" example: http://react.semantic-ui.com/modules/dimmer#simple-dimmer. That should be enabled by virtue of having a parent with
ui dimmable dimmed. See the SUI-core docs: http://semantic-ui.com/modules/dimmer.html#simple-dimmerDigging into it further, two things stuck out to me:
visibleclass, regardless of whether it was active.activecontrols the opacity,visiblesets thevisibility.disabledis a prop that overrides any active/visible state so we shouldn't be using it for that.This fixes both issues.
visibleis only added when the dimmer is active, which removes the need to forcedisabled, which enables clicking on children and fixes the "simple" usage.disabledas a prop which conforms more to our API. A user can set that to override any active/visible state.