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.

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

fix(Dimmer): allow children to be clicked #782

Merged
merged 3 commits into from
Nov 2, 2016

Conversation

ben174
Copy link
Contributor

@ben174 ben174 commented Nov 2, 2016

Fixes #780 by adding a disabled class when the Dimmer is not active. Also adds a test to ensure the fix isn't regressed.

@codecov-io
Copy link

codecov-io commented Nov 2, 2016

Current coverage is 99.49% (diff: 100%)

Merging #782 into master will not change coverage

@@             master       #782   diff @@
==========================================
  Files           136        136          
  Lines          2185       2185          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           2174       2174          
  Misses           11         11          
  Partials          0          0          

Powered by Codecov. Last update f4e1495...4791166

@layershifter
Copy link
Member

We need asserts for disabled there, too.

@ben174
Copy link
Contributor Author

ben174 commented Nov 2, 2016

Good point, I just tried creating such assertions while disabled and am getting false positives. I've tested it in the wild and am sure it's working correctly, but the test seems to be simulating a click directly against the anchor and bypassing the overlying element.

Do you know of a way to simulate an actual click on the anchor as if a user was clicking on the DOM, rather than clicking on the element itself?

@@ -26,6 +26,13 @@ describe('Dimmer', () => {
shallow(<Dimmer content={text} />)
.should.contain.text(text)
})

it('renders clickable buttons', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This test is already covered by common.rendersChildren() above. We can remove this one.

@levithomason
Copy link
Member

The click simulation is not subject to z-index blocking effects like a user is. You can simulate an event on any node that can be accessed programmatically. The event is dispatched via a reference to the node, unlike a user event which is the result of the cursor position and top most element.

Instead, add a test that asserts the disabled className is added when active is false, and another that asserts disabled is not present when active is true. I did not test this, but it should be something like this:

describe('active', () => {
  it('removes the `disabled` className when true', () => {
    shallow(<Dimmer active />)
      .should.not.have.className('disabled')
  })
  it('adds the `disabled` className when false', () => {
    shallow(<Dimmer active={false} />)
      .should.have.className('disabled')
  })
})

Rather than simulating click on an anchor, this test checks to ensure
the disabled class is properly applied according to the active state.

Also removes a test which simulated a click directly to an anchor within
a disabled Dimmer since it was already covered elsewhere.
@ben174
Copy link
Contributor Author

ben174 commented Nov 2, 2016

Makes sense, I was trying to write something that actually validates that the item is clickable, rather than trusting that the disabled class fixes it. But I suppose tests covering the functionality of disabled elsewhere in the suite would be a more appropriate place for that.

@levithomason
Copy link
Member

Correct, really the sizing would be a CSS test since it is testing what the result of the CSS rule is for that HTML class. We only want to assert that we are generating the correct markup.

@levithomason levithomason merged commit 288b03b into Semantic-Org:master Nov 2, 2016
@levithomason
Copy link
Member

Released in [email protected], thanks much!

@ben174 ben174 mentioned this pull request Nov 2, 2016
layershifter pushed a commit that referenced this pull request Nov 9, 2016
* fix(Dimmer): allow children to be clicked

* test(Dimmer): assert disabled class is applied only when active

Rather than simulating click on an anchor, this test checks to ensure
the disabled class is properly applied according to the active state.

Also removes a test which simulated a click directly to an anchor within
a disabled Dimmer since it was already covered elsewhere.

* style(Dimmer): remove white space
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dimmer - Inactive dimmer still overlays segment contents, making them unclickable
5 participants