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

feat(Dropdown): support disabled items #482

Merged

Conversation

jeffcarbs
Copy link
Member

@jeffcarbs jeffcarbs commented Sep 9, 2016

Addresses #478

I added code that should handle ignoring a click on the dropdown item and keeping the menu open. However, the onClick is never getting called so the event is bubbling to the Dropdown element itself which is toggling it's visibility.

Is there some magic happening somewhere that disables clicks on an element with disabled passed as props? As you can see, it's not being applied onto the element. It's just being used to add the disabled class.

EDIT AHH it's pointer-events: none; 😞 . Thoughts on how to handle this? Two ideas:

  • manually enable pointer-events via inline style
  • in the dropdown handleClick function, check if the click was within the menu (can you even do that with synthetic events?) and ignore it.

I don't love either of those idea, tbh.

@dylankiss
Copy link
Contributor

@jcarbo Your second option can be achieved by listening on the <DropdownMenu /> component, I guess. It is the parent containing all <DropdownItem /> components and would receive the click event first.

@levithomason
Copy link
Member

We can detect click inside/outside of a node, but this requires a ref and reading the DOM. Also, there are several elements that could be inside of the item. The event target could be any one of them. We'd have to apply the disabled prop as a div attribute as well, since we wouldn't have access to the original virtual DOM item's props. Then we could check the event target and traverse its parents looking for .item[disabled], which if found, stop immediate propagation.

As much as I don't like to do so, I think we add the inline style. Then, we can simply add the style in renderOptions() and in handleItemClick() add item.disabled as a condition to stop immediate propagation:

// renderOptions() map() Dropdown.Item
style={{ ...opt.style, pointerEvents: 'all' }}

// handleItemClick()
const item = this.getItemByValue(value)

if (multiple || item.disabled) {
  e.nativeEvent.stopImmediatePropagation()
}

@levithomason
Copy link
Member

Inline comments are much appreciated anywhere there is odd or special case handling like this. Thanks in advance.

@jeffcarbs jeffcarbs force-pushed the feature/dropdown-item-disabled branch from 4b2ea2b to 1b448bd Compare September 9, 2016 16:30
@codecov-io
Copy link

codecov-io commented Sep 9, 2016

Current coverage is 98.63% (diff: 100%)

Merging #482 into master will increase coverage by 0.01%

@@             master       #482   diff @@
==========================================
  Files           101        101          
  Lines          1453       1465    +12   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1433       1445    +12   
  Misses           20         20          
  Partials          0          0          

Powered by Codecov. Last update 974f2a4...fa7de43

@jeffcarbs
Copy link
Member Author

Thanks for the feedback.

I've implemented the inline-style option. As much as I hate inline styles, it was significantly simpler than dealing with the DOM. Clicking a disabled item now just does nothing and keeps the menu open.

Let me know if there's anything else this is missing.

title='Disabled Item'
description='A disabled dropdown item does not allow user interaction'
examplePath='modules/Dropdown/States/DisabledItem'
/>
Copy link
Member

Choose a reason for hiding this comment

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

Let's update the Disabled description to include "menu or item" and drop the title/description on the disabled item example. This way they show as two examples under one heading:

image

@levithomason
Copy link
Member

Left some inline comments. If we could get a test that asserts disabled items can never be selected, then we can forgo the test asserting they aren't made active on Enter. Since, we'd also need to ensure they aren't made active on blur.

@jeffcarbs jeffcarbs force-pushed the feature/dropdown-item-disabled branch from 6b47f45 to fa7de43 Compare September 9, 2016 18:03
@jeffcarbs
Copy link
Member Author

Implemented the remaining feedback, namely preventing disabled elements from being selected. If all elements are disabled, the selectedIndex is undefined which prevents things like selection.

Actually found a bug and another broken test: keeps the selection within the range of remaining options. The logic for keeping the selection in-bounds happens before the value is persisted to state, so the most recently selected item isn't filtered out of the options returned by getMenuOptions. I wound up fixing the bug and updating the logic to take the disabled state into account. So it will keep you in bounds at the next non-disabled item.

@levithomason levithomason merged commit e0f643f into Semantic-Org:master Sep 9, 2016
@levithomason levithomason changed the title Dropdown: Disable dropdown item feat(Dropdown): support disabled items Sep 9, 2016
@levithomason
Copy link
Member

Thanks much, released in [email protected].

@jeffcarbs jeffcarbs deleted the feature/dropdown-item-disabled branch September 9, 2016 20:05
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.

4 participants