New dropdown #2285
New dropdown #2285
Conversation
|
this one looks good except several naming questions |
|
and one small css problem |
| let nextState = { open: false, selectedValue: this.state.selectedValue }; | ||
|
|
||
| if (item.value !== this.state.selectedValue) { | ||
| if (!stateless) { |
There was a problem hiding this comment.
as i could understand this prop is for dropdowns that do not change it's selected value on item click
looks like it's some kind of triggering actions on item select and updating it's value asynchronously
so, maybe it's better to rename it to something more action-like. stateless is more about internal implementation, not usage purpose
There was a problem hiding this comment.
or if it for actions only, we can pull out separate ActionsDropdown component based on this one
There was a problem hiding this comment.
Well, stateless just means that TextDropdown must not change its value and only use props.value. It is just one condition, and I think separate Dropdown will be an overhead in this situation.
stateless could be renamed though.
| * This component is not responsible for different skins, TextInputs, infinite lists, or any other complex stuff. | ||
| * If you need any functionality beyond existing, try different Dropdown or build new one. | ||
| */ | ||
| export default class TextDropdown extends Component { |
There was a problem hiding this comment.
TextDropdown sounds like you can operate on some text of the component, e.g. have some input for it.
if it's a general-purpose dd maybe we can call it just Dropdown?
There was a problem hiding this comment.
It is not general purpose, nobody will extend it. It could be SimpleDropdown or something like that.
| type Props = { | ||
| /** An array of all possible values which will be in a list */ | ||
| // $FlowFixMe | ||
| items: Array<Item | InternalItem | string>, |
There was a problem hiding this comment.
is there any usage in the code of InternalItem notation?
There was a problem hiding this comment.
It is used for local state, just for code readability (item[1] vs. displayText for example).
|
|
||
| describe('SearchDropdown', function() { | ||
| // @todo sinon and promises | ||
| // it.skip('should set list only for corresponding token', function() { |
There was a problem hiding this comment.
are that comments intentional?
There was a problem hiding this comment.
Yes. We should combine sinon with promises somehow to test them. For this component it is critical, because many testcases are about promises resolve.
| * It is possible to define some initials: value, displayText and items. | ||
| * But after mounting component handle these values monopoly via state. | ||
| */ | ||
| export default class SearchDropdown extends Component { |
There was a problem hiding this comment.
not sure that this implementations makes sense. we have only one place where this one is used and i think it can be replaced with some other component like Typeahead
i think SearchDropdown makes more sense to use as a filtering dd w/o any fetching functionality, e.g. you type some text in the input and initialItems are filtered with this value (like one for taxonomies with highlighting matches).
There was a problem hiding this comment.
Well, it is just proof of concept (that functionality split to SmartList works well). Also I tried do not change UI because this pr is about refactoring, not new features or design update.
| * This component knows how to position itself around pivot element and fit to the screen. | ||
| * Also component knows how to handle scroll and keyboard events. | ||
| */ | ||
| export default class SmartList extends Component { |
There was a problem hiding this comment.
again, this name is more about implementation, not it's purpose
maybe it's better to call it DropdownList if we wanna use it for all our DDs?
There was a problem hiding this comment.
Yeah, DropdownList is good.
| @@ -13,10 +13,9 @@ import { stateToMarkdown } from 'draft-js-export-markdown'; | |||
|
|
|||
| // components | |||
| import { ContentBlock, ContentState, Editor, EditorState, RichUtils } from 'draft-js'; | |||
| import { Dropdown } from '../dropdown'; | |||
| import FontDropdown from './font-dropdown'; | |||
There was a problem hiding this comment.
isn't it possible to use TextDropdown here?
There was a problem hiding this comment.
No, because it has different styling and simplier logic.
| @@ -81,6 +83,14 @@ module.exports = { | |||
| ], | |||
| }, | |||
| { | |||
| name: 'Dropdowns', | |||
| components: () => [ | |||
| // path.resolve(__dirname, '../src/components/core/dropdown/smart-list/smart-list.jsx'), | |||
There was a problem hiding this comment.
commented intentionally?
There was a problem hiding this comment.
Well, I think I should remove it from here: there is no place for SmartList in styleguide.
| align?: 'left' | 'right', // @todo | ||
| /** Base element: list will try to stick around it. E.g. dropdown current value box. | ||
| * Default value: previous sibling or parent. */ | ||
| pivot?: HTMLElement, |
There was a problem hiding this comment.
maybe it's just for me, but pivot is very confusing as it's often used for excel-like tables/components
There was a problem hiding this comment.
Well, any word used somewhere) But lets invent a better one)
| type Props = { | ||
| /** An array of all possible values which will be in a list */ | ||
| // $FlowFixMe | ||
| items: Array<Item | InternalItem | string>, |
There was a problem hiding this comment.
i'd think about using one style for items prop. i couldn't find any usage of InternalItem in client code and not sure there are a lot of usages of Array<string>
No description provided.