Skip to content

feat(splitter): splitter component #53

Merged
merged 50 commits into from
Oct 18, 2018
Merged

Conversation

anrodkin
Copy link
Contributor

No description provided.

@anrodkin anrodkin changed the title [WIP] feat(splitter): splitter component feat(splitter): splitter component Oct 16, 2018
@anrodkin anrodkin changed the title feat(splitter): splitter component [WIP] feat(splitter): splitter component Oct 16, 2018
@anrodkin anrodkin changed the title [WIP] feat(splitter): splitter component feat(splitter): splitter component Oct 16, 2018
src/lib/splitter/_splitter-base.scss Outdated Show resolved Hide resolved
src/lib/splitter/gutter.directive.ts Outdated Show resolved Hide resolved
src/lib/splitter/splitter.component.ts Show resolved Hide resolved
src/lib/splitter/splitter.component.ts Outdated Show resolved Hide resolved
Default = 'default'
}

export const enum SizeProperty {
Copy link
Member

Choose a reason for hiding this comment

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

используется только в директиве, ENUM и нужно там объявить

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Используется в 2-х директивах, поэтому предлагаю оставить её здесь

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Готово

src/lib/splitter/splitter.constants.ts Outdated Show resolved Hide resolved
src/lib/splitter/splitter.interfaces.ts Outdated Show resolved Hide resolved
src/lib/splitter/splitter.scss Outdated Show resolved Hide resolved
commitlint.config.js Outdated Show resolved Hide resolved
Height = 'height'
}

export const enum State {
Copy link
Member

Choose a reason for hiding this comment

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

хотим ли мы экспортировать данные enum?
если нет, то давай уберем.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Там линтер ругался на то, что чать экспортилась, а часть нет. Поэтому и поставил везде export

Copy link
Contributor Author

Choose a reason for hiding this comment

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

готово

src/lib/splitter/splitter.component.html Outdated Show resolved Hide resolved
src/lib/splitter/splitter.component.ts Outdated Show resolved Hide resolved
src/lib/splitter/splitter.component.ts Outdated Show resolved Hide resolved
src/lib/splitter/splitter.component.ts Outdated Show resolved Hide resolved
src/lib/splitter/splitter.component.ts Outdated Show resolved Hide resolved
src/lib/splitter/splitter.component.ts Outdated Show resolved Hide resolved
src/lib/splitter/splitter.component.ts Outdated Show resolved Hide resolved
src/lib/splitter/splitter.component.ts Outdated Show resolved Hide resolved
src/lib/splitter/splitter.component.ts Outdated Show resolved Hide resolved
@lskramarov
Copy link
Contributor

еще нужно добавить overflow: hidden что бы при уменьшении размера ничего не "вываливалось"

  • нужно добавить пример с вложенным сплиттером

@mikeozornin
Copy link
Contributor

mikeozornin commented Oct 17, 2018

Сам сплиттер

Я понимаю, что делали без макетов. У сплиттера не хватает какого-то хувера кроме изменения курсора. Хочется чего-то более заметного. Сейчас есть изменение цвета: http://mosaic.ptsecurity.ru/4.1.2/examples/example-example25/index.html. С ним лучше, чем без него.

Мне кажется в спилиттере должно быть поменьше дизайна, даже имеющаяся серая полоса может подойти не всем. И иконка не должна быть обязательна, но это тоже вопрос к тому, что делалось без дизайна.

Оба пункта выше можно влить как есть и поменять позже.

Мы обсуждали, что нужно бы проверить, как будет вести себя контент в областях, если контент будет сложный, типа грида. Удалось что-то проверить? Это влияет на вид и работу самого сплиттера. Если контент будет долго перерисовываться, придется делать при перетаскивании сплиттера рисовать его фантом, а изменения размеров производить после отпускания.

При резком изменении сплиттера с уменьшением меньше минимальной границы фрейм который не ресайзится скачет. При плавном изменении скакания почти незаметны. Кейс не очень частый, поэтому если по-другому не выйдет, то пункт готов снять. Вот скринкаст: http://d.mikeozornin.ru/eRAJIP

Доки

Весь контент доков прилип к краям, нужен какой-то отступ.
@fost может пора сделать какой-то шаблон для страницы доков?

Splitter is designer to use
Кажется тут что-то с текстом. Может designer → designed?

Давай у последнего примера «min-width for the first area» этот самый min-width уменьшим вдвое? Ширина слишком большая Сейчас постоянно думаю, что баг, а потом вспоминаю об ограничении.

Возможно это простой и нереальный пример, но можно сделать вот так:
http://screenshots.ptsecurity.com/mozornin-2018-10-17_12-32-04.png

А можно сделать пример, где будут оба вида сплиттеров одновременно? Как в текущих доках: http://screenshots.ptsecurity.com/mozornin-2018-10-17_12-36-48.png

Copy link
Contributor

@mikeozornin mikeozornin left a comment

Choose a reason for hiding this comment

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

Там я комментарии я написал. Сам компонент можно влить, если технических проблем при ресайзе не будет.

@anrodkin
Copy link
Contributor Author

@lskramarov

  1. Убрал CSS классы, зиаспользовал напрямую свойства
  2. Убрал лишний код
  3. Добавил overflow: hidden
  4. Добавил пример с вложенными сплиттерами

@mikeozornin

  1. Добавил hover
  2. Добавил примеры с кастомным стилем
  3. В качестве сложного контента проверил с navbar. Отрисовка при изменении размера порядка 2-3 мс.
  4. Пофиксил проблему c дерганьем при резком скачке
  5. Уменьшил min-width до 200 px
  6. Пофиксил проблему, когда размер области не уменьшался до 0 при быстром уменьшении
  7. Добавил отступы в доки

@mikeozornin mikeozornin self-requested a review October 18, 2018 13:30
@pimenovoleg pimenovoleg merged commit 341c3b6 into positive-js:master Oct 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants