IW-1244 | Extending dropdowns with new props #77
Conversation
There was a problem hiding this comment.
Few comments - mostly minor ones, but the complexity of this feels like a code smell.
Do you really need the Dropdown component to handle all that different situations? (ie. does the one Dropdown instance need to handle them)
In general it looks like we have WAY TOO MANY styles/behevious for a simple dropdown (that should be unified) - just the shadow has 3 different settings!
Also please remember that props !== classNames - limit the props if those are unneccessary.
Consider splitting Dropdown with different behaviors into different components, maybe to look like this:
- Dropdown
- DropdownLabel
- DropdownItem
- NestedDropdown
- DropdownLabel
- DropdownItem
- StickedDropdown
- DropdownLabel
- DropdownItem
|
@vforge I rewrote the dropdown internals and improved the API, cutting out some props we did not really need. Current implementation is compliant with the Ember one + it covers all cases we need. As for your idea to create multiple derrivatives of the main dropdown, I see only one possible derrivative - NestedDropdown, but it would still need to return Dropdown but with specific props set inside of it. I'd say: let's merge the current dropdown (we're testing it as we develop it as we use it in Feeds) and then if we need more specific cases in the future, let's add them. What do you think? |
source/components/Dropdown/index.js
Outdated
|
|
||
| if (shouldPreventDefault) { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); |
There was a problem hiding this comment.
Why do we need stop propagation?
There was a problem hiding this comment.
To prevent from running the handleClick() two times if the click takes place inside the toggle.
source/components/Dropdown/components/DropdownToggle/__snapshots__/index.spec.js.snap
Outdated
Show resolved
Hide resolved
| }); | ||
| var toggleContentComponent; | ||
|
|
||
| if (typeof toggleContent === 'function') { |
There was a problem hiding this comment.
Do we want to support jsx toggle content as well?
<Dropdown toggle={<div>my toggle</div>}>
Or function and string is enough? :D
There was a problem hiding this comment.
JSX is the else in this case :)
| ); | ||
| }; | ||
| class DropdownToggle extends React.Component { | ||
| static getToggleContentComponent(toggleContent, isLevel2) { |
There was a problem hiding this comment.
I'd suggest isNestedLevel or anything without number in it
| </React.Fragment> | ||
| ); | ||
| } else { | ||
| toggleContentComponent = toggleContent; |
There was a problem hiding this comment.
I see no reason in accepting all three types, as this one can be merged with function one with () => <div />.
I'd check for === string and assume toggleContent is function in other case. docs will do the rest
There was a problem hiding this comment.
This is actually the most common case -> passing JSX as component toggle. Again - we enable this possiblity to be compliant with ember's design-system which accepts all three possibilities.
source/components/Dropdown/index.js
Outdated
|
|
||
| onMouseLeave() { | ||
| const { isTouchDevice } = this.state; | ||
| handleClick(shouldPreventDefault, e) { |
There was a problem hiding this comment.
why do we need shouldPreventDefault flag?
There was a problem hiding this comment.
To prevent from running the handleClick() two times if the click takes place inside the toggle.
bkoval
dismissed
vforge’s stale review
This pull request is hanging for some time with no additional feedback and has 2 approves already :)
7 participants
@Wikia/iwing