Skip to content

Multiselect#87

Merged
serjout merged 18 commits intosonm-io:GUI-01-prepare-your-afrom
maestrow:GUI-01-prepare-your-a
May 3, 2018
Merged

Multiselect#87
serjout merged 18 commits intosonm-io:GUI-01-prepare-your-afrom
maestrow:GUI-01-prepare-your-a

Conversation

@maestrow
Copy link
Copy Markdown
Member

No description provided.

.multiselect {
&__container {
display: flex;
width: fit-content;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

это нестандартная штука. она заведомо несовместима со всеми браузерами. поэтому лучше убрать.

@@ -0,0 +1,58 @@
@import 'guide.less';

@Multiselect-ListContainer-MaxHeight: 200px;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

у нас принят camelСase во всем кроме именования классов css ))

button: propTypes.string,
popup: propTypes.string,
}),
bgcolor: '',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

как-то не очень эта штука выглядит по след. соображениям:

  1. создается впечатления что этих цветов несколько, а по факту он один.
  2. ломается механизм cssClasses
    и имя bgcolor не похож на camel case ))

там сейчас в less файле есть переменная @dropdownInputBgColor кстати.
предлагаю завести css custom property там же

:root {
    --dropdown-input-button-bg-color: @dropdownInputBgColor;
}

.dropdown-input__botton {
    background: var(--dropdown-input-button-bg-color);
}

и все. при необходимости перекрасить кнопку - это можно будет в css того компонента который использует внутри себя этот дропдаун

соответсвенно js код компонента менять вообще не надо

@@ -0,0 +1,4 @@
<svg fill="currentColor" height="24" viewBox="0 0 24 24" width="24" xmlns="http://www.w3.org/2000/svg">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

можт в Icon добавить эту иконку? Вдруг еще где пригодится

сейчас эта иконка спрятана в компоненте, при таком раскладе можно обойтись и без экспорта svg в js. а просто в css задать кнопке background image

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Если иконка не будет внутри папки компонента, а будет в Icons, то в этом случае также можно будет импортировать ее в js, препятствий не будет. Я делал

<div class="close-button"></div>
.close-button {
    color: green;
    background: url(ссылка на иконку);
}

Иконка зеленой не стала. Пробовал color указывать и у родителя, тоже никакого эффекта. Может надо background не ссылкой, а background: url('data:image/svg+xml;utf8,<svg ...> ... </svg>'); и тогда получится?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

если svg в background то она расценивается как просто картинка. к ней color: green; не применяется.

если же svg является частью DOM то применяется currentColor.

поэтому давайте добавим крестик в components/common/Icon

> {
public static readonly defaultProps: IMultiSelectOptionalInputProps = {
label: '',
throttleTime: 100,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

throttle и debounce таки немного разные штуки. лучше переименовать чтобы не запутаться в терминологии. ну это я немного придираюсь).

кстати это можно вообще const вверху модуля описать. не думаю что кто-то будет это менять

this.props.onRequireClose && this.props.onRequireClose();
};

protected onClearClick = () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

onДействиеГде

если где-то встерчается onButtonClick - значит я накосячил )

);

const StateInfo = observer(() =>
<div>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

это прикольная штука ))

name={value}
title={name}
value={checked}
onChange={this.onChangeCheckbox.bind(this, value)}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this.onChangeCheckbox.bind(this, value) - на каждой итерации перерисовки создаётся новая функция и оптимизация PureComponent не работает

</div>

<div className="multiselect__list-container">
{filteredList.map(this.createListItem.bind(this))}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

тоже странная штука.

  1. bind тут неоптимально используется - проще было createListItem (и кстати тогда уж renderListItem) один раз при создании интстанса забиндить к this:
    createListItem = (item: T) => { ....... }

  2. тут вообще все можно упростить

 <div className="multiselect__list-container">
    <filteredList.map((item: T) => 
        <Checkbox 
               value={checked or not} 
               name={get(this.props.dataIndex, item)} 
               onChange={this. onChangeCheckbox} />)
protected onChangeCheckbox = (
        params: ITogglerChangeParams,
) => {
...
params.name - тут будет имя item'a
...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Bind перенес в конструктор, но от функции избавляться не стал, т.к. без нее вызов getValue будет происходить дважды. Ну и (имхо) с функцией более читаемый код, удобнее навигация. По onChangeCheckbox изменения сделал.

@maestrow maestrow closed this May 3, 2018
@maestrow maestrow reopened this May 3, 2018
@serjout serjout merged commit b75e396 into sonm-io:GUI-01-prepare-your-a May 3, 2018
@maestrow maestrow deleted the GUI-01-prepare-your-a branch May 3, 2018 10:38
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.

2 participants